Repository navigation
Feature image occlusion - #2367
Conversation
|
Nice work, Mani! Will take a closer look at this later today/tomorrow, but just wanted to say for now: Glad to see you back! For continuity's sake, here are the discussions/iterations up to this point: krmanik#1 |
|
Trying to fix these errors but I am not able to figure out the issues. |
|
Wow, great job! You usually don't need to construct those request types when calling backend routines. So instead of: request = image_occlusion_pb2.ImageClozeMetadataRequest(path=path)
return self._backend.get_image_cloze_metadata(request)Try: return self._backend.get_image_cloze_metadata(path=path)I assume you modeled your code after |
|
Thank you so much for working on this Mani! I've only had a brief play with it so far, but some initial thoughts:
Some thoughts on design decisions: Storing the occlusion data in the fields instead of images is great. Storing them as HTML, I'm a little less sure about. Because users can't see the paths unless they enable the HTML editor, I fear some users may accidentally damage or delete their occlusions when they edit the text (point 1 on krmanik#1 (comment)). We could partly avoid that by changing the editor so that users aren't able to edit the field, though that won't solve the problem for older clients/AnkiWeb/etc. Would it make sense to make the paths visible instead? Eg instead of something like What if we used something like this instead? (omitting values if they're the default) We could then adjust cloze.rs to look for image-occlusion as a prefix, and transform the text into the desired HTML for both the question and answer side. What do people think? Would that be an improvement, or make things worse? One other thing - the script in the notetype. It's impressive how simple it is, but I think storing it in the notetype will make it hard for us to make changes in the future if we have old notetypes to worry about, and it increases the chances the user will make accidental changes. Maybe our reviewer TypeScript could export the required function(s) instead, so we could keep the code in the notetype to the minimum? |
|
@dae I started jotting down some notes yesterday, and your points echo a lot of my thoughts 👍
Agreed, yes. I appreciate the effort you went through in adding all of these, Mani, but I do think we should keep the UI complexity down. Even the rectangle tool by its own will likely cover 90% of all IO use cases. With the ellipse tool and path tool by its side, I think we have all bases covered. Being lean here is particularly important on mobile I would argue, where selecting the right tool via touches is likely more of a challenge than on desktop (though even on desktop, in playing around with the PR over the past couple of days I've already had multiple instances where I ended up confusing square vs rectangle and circle vs ellipse, leading to momentary confusion). So from a UX and maintenance standpoint, I would vote to not include triangle, square, and circle. Though again, thanks a lot for your work on these, Mani! For the rest, focusing on note type design decisions for now as I don't want to make this comment overlong:
Last, some considerations towards editing existing notes and handling multiple IO note types:
|
It works. Thanks.
Some of the tools will be removed and only selected tools be available.
I have implementing considering mobile version, but notes editor will be also ported similar to other web pages. So, I think I should look into integrating into the existing note editor(but currently separate note editor for mobile version should be used).
The editing of card is not available, the edit button should open mask editor and generate fabric.Canvas objects and place on mask editor. After finish of edit, the data in cloze re-adjusted. (design idea)
I will change the zoom tools to always show.
It is reverse of what fill tools works. In my (incorrect) implementation first select shapes and then select fill tools to change the color of shapes. But it should work like select fill tools and select any shapes and change their fill-color. It will be fixed in next commits.
I think it will reduce the html in the notes. This is information visible to users. I will look into cloze.rs for implementations. As pointed by glutanimate,
Because it checks for hidden and cloze (css) class to show and hide the shapes. So the following cloze should be replaced with And generated html using later part Or may be other cloze generation should be used.
It will be best to export this to reviewer TypeScript, because minimum changes in image occlusion note type make it easier to updates.
This can be implemented similar to cloze, when cloze note types selected then other cloze button are shown. So, here the image selection to generate image occlusion should show/hide the button depending on note types. If note types are image occlusion then show the button to select image and show mask editor otherwise hide it. Also three icon button will be shown when note types
Edit scenario
|
9c4fbd0 to
6581d04
Compare
|
The latest commit is not complete. I have tried to filter out the data-cloze, but need help to finish it. I want the data-cloze should only contains image-occlusion, but filtering is not done. |
|
We can bypass all the usual cloze rendering and have complete control over what is output for image occlusion. Eg: diff --git a/rslib/src/cloze.rs b/rslib/src/cloze.rs
index 720f89e74..158cd2f2d 100644
--- a/rslib/src/cloze.rs
+++ b/rslib/src/cloze.rs
@@ -140,9 +140,12 @@ impl ExtractedCloze<'_> {
buf.into()
}
- fn image_occlusion(&self) -> Cow<str> {
- get_image_cloze_data(&self.clozed_text()).into()
- }
+ /// If cloze starts with image-occlusion:, return the text following that.
+ fn image_occlusion(&self) -> Option<&str> {
+ let Some(first_node) = self.nodes.get(0) else { return None };
+ let TextOrCloze::Text(text) = first_node else { return None };
+ text.strip_prefix("image-occlusion:")
+ }
}
fn parse_text_with_clozes(text: &str) -> Vec<TextOrCloze<'_>> {
@@ -217,6 +220,10 @@ fn reveal_cloze(
) {
let active = cloze.ordinal == cloze_ord;
*active_cloze_found_in_text |= active;
+ if let Some(image_occlusion_text) = cloze.image_occlusion() {
+ buf.push_str(&render_image_occlusion(image_occlusion_text, question, active));
+ return;
+ }
match (question, active) {
(true, true) => {
// question side with active cloze; all inner content is elided
@@ -235,9 +242,8 @@ fn reveal_cloze(
}
write!(
buf,
- r#"<span class="cloze" data-cloze="{}" {} data-ordinal="{}">[{}]</span>"#,
+ r#"<span class="cloze" data-cloze="{}" data-ordinal="{}">[{}]</span>"#,
encode_attribute(&content_buf),
- cloze.image_occlusion(),
cloze.ordinal,
cloze.hint()
)
@@ -246,8 +252,7 @@ fn reveal_cloze(
(false, true) => {
write!(
buf,
- r#"<span class="cloze" {} data-ordinal="{}">"#,
- cloze.image_occlusion(),
+ r#"<span class="cloze" data-ordinal="{}">"#,
cloze.ordinal
)
.unwrap();
@@ -265,8 +270,7 @@ fn reveal_cloze(
// question or answer side inactive cloze; text shown, children may be active
write!(
buf,
- r#"<span class="cloze-inactive" {} data-ordinal="{}">"#,
- cloze.image_occlusion(),
+ r#"<span class="cloze-inactive" data-ordinal="{}">"#,
cloze.ordinal
)
.unwrap();
@@ -283,6 +287,11 @@ fn reveal_cloze(
}
}
+fn render_image_occlusion(text: &str, question_side: bool, active: bool) -> String {
+ // todo: alter HTML depending on question/answer side, active/non-active cloze
+ get_image_cloze_data(text)
+}
+
pub fn reveal_cloze_text(text: &str, cloze_ord: u16, question: bool) -> Cow<str> {
let mut buf = String::new();
let mut active_cloze_found_in_text = false;Does that make things easier? |
|
(we don't even need to use |
|
Thanks I have used the patch and it is easier then earlier implementation. I have added new notes type for image cloze in Now adding the following to Occlusion fields and image in Image tag with |
|
The new card rendering approach and note type format look clean, nice work! A couple of thoughts regarding
|
7bf0e0b to
d56ea36
Compare
|
Included the mask editor in note editor. The editor have toggle button to switch between note editor and mask editor. Next implementation todo, add a button in mask editor's top toolbar |
|
I'll have more time to return to this in a day or so, but just a quick note for now: it might be nicer to keep I/O in a separate screen/page for the initial implementation, as that will make it easier to integrate into the mobile clients. There's a bunch of work that needs to be done before we can integrate the editor into the mobile clients, so combining the two would mean there'd be a longer wait for mobile users to get access to this. |
|
The implementation for mobile will be done using |
|
Just trying to keep things simple to start with :-) If we have two different ways of invoking/using the I/O editor depending on platform, that's more variables we need to test. Integration inside the editor is the best way in the long run, but I just thought it might make more sense to focus on the external-case for now, and then when the mobile clients have adopted the new editor, we can then switch the implementation over. But I don't have strong feelings here if you're keen to work on both implementations. |
This is most requested feature for AnkiDroid and AnkiMobile users. This feature is implemented like import-csv features.
Only image is stored in
collection. Media, the question & answer masks are stored in notes itself, usingdata-*attributes and drawn using canvas api in reviewer.Mask Editor
fabricjsandpanzoom.Note editor
Very simple text editor is implemented (still needs improvement)
Image