Feature image occlusion #2367
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
|
krmanik
force-pushed
the
feature-image-occlusion
branch
from
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
|
krmanik
force-pushed
the
feature-image-occlusion
branch
from
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. |
|
I will separate it into two PR, in this PR only for mobile client change will be added. In other PR, I will add the changes for adding into note editor (and works on the implementation). |
krmanik
force-pushed
the
feature-image-occlusion
branch
from
46fbd81
to
634b2a2
Compare
|
Is this the difference between c1,c2,c3 vs c1,c1,c1? Could you perhaps avoid any extra text on the field and just guess which type it is based on the cloze numbers? |
It is different because in first case c1,c2,c3 is individual shapes and three notes. In later case c1,c1,c1 is group cloze, one note, all shapes are combined into one group and all shapes drawn once. For, e.g.
For guessing part I don't know it can be done without storing the hideall value somewhere. Then values may be added to first cloze. -{{c1::image-occlusion:rect:left=19.54:top=46.96:width=126.13:height=53.29:fill=#55aaff:quesmaskcolor=#f06}}
+{{c1::image-occlusion:rect:left=19.54:top=46.96:width=126.13:height=53.29:fill=#55aaff:quesmaskcolor=#f06:hideall=true}} |
|
Ok, I think I get it. We want to change whether inactive clozes are hidden or not on the question side, but not change how many cards are generated? And c1,c1,c1 would be equivalent to "hide all, guess all", not "hide all, guess one". What if we put a hideinactive property on every cloze, and not just the first one? That would allow for a 'hide some, guess one' option in the future if we ever wanted to add such a feature, and it simplifies the cloze rendering, since all the information is available directly inside each cloze. Would that make sense? (@glutanimate may have further thoughts) |
This approach is good and easy. Thanks |
Now existing notes can be edited. For now only shapes and notes can be edited. Deck selection and image changes can not be done. On mobile client before loading web pages the path to image or note id can be passed. The page is built for mobile client so may be following steps can be used to test it.
|
There was a problem hiding this comment.
Looks like it's coming together really well Mani :-)
I was initially confused that there's no icon in the editor anymore, then I figured out you meant that you can only test via the external browser/that code currently. I also initially thought testing on mobile would be hard due to sandboxing, then realised the path to the image is on the computer host :-)
A few things I noticed:
- I used 'hide all, guess one', and both of the rects I created had a quesmarkcolor set. I played around with color setting in the past, so not sure if that's being remembered or is the default. But when it comes time to review, both rectangles have the same color, so you can't tell which one is being asked. If I remove quesmaskcolor manually, the active rect changes color.
- I tried editing a note, and got:
lib.ts:199 Uncaught TypeError: import_fabric.default.Rect is not a constructor
at lib.ts:199:34
at Array.forEach (<anonymous>)
at generateShapeFromCloze (lib.ts:173:15)
at mask-editor.ts:78:13
at fabric.js:21623:19
at HTMLImageElement.onLoadCallback (fabric.js:1100:30)
- Would it make sense to also show hide one/all buttons in the image tab, and not just the notes tab? @glutanimate what percentage of occlusions would you say have added text content?
- I find myself wanting a 'resize to fit' button more than 'show at 100% size' - maybe it could toggle, or there could be both?
- If you add 4 rects, it says '4 notes added' - perhaps it should say '4 cards added' instead?
- I tested this in Mobile Safari, and was successfully able to add an occlusion. The basic creation of shapes, zooming, panning using the pan tool, etc all worked for me - it's already looking great. Pinching to zoom created a shape instead of zooming, which was a little surprising, but that may be because I was testing with a mouse and an emulated pinch gesture.
- When I tested with a larger image, I unfortunately hit a problem: the screen fails to load, and Safari prints the following warning in the console:
Canvas area exceeds the maximum limit (width * height > 16777216)(more info: https://pqina.nl/blog/canvas-area-exceeds-the-maximum-limit/). Presumably it can be worked around by applying a constant scaling factor to the image or canvas coordinates or enforcing a maximum height/width on images prior to editing, but it unfortunately means a more complicated implementation. - In some of the other screens like the deck options, they look for a
#testsuffix on the page name to automatically set up for testing, so you don't need to run any commands from the console. You could optionally do something similar here - for exampleimage-occlusion.html#test-/home/user/foo.pngcould automatically call anki.setupImageOcclusion("/home/user/foo.png"). That makes it a bit easier to test, as you don't need to run console commands, especially on mobile clients. Could also optionally do#testedit-123123123for the editing case.
Here are two things to consider, during review of image cloze notes
Instead importing
I have added option as drop down menu to change canvas size from
I have added strings to ftl and implemented it.
It is fixed now. I have used suggested implementation. Thanks
I have updated it, so to test we have to just load test url now now. Also some changes needs to do for smaller screen size ( I will update it). |
Makes it clearer that only one of them can be returned
The second unwrap should be ok, as the input is utf8
- fixes crash when note doesn't exist - Ok(None) case was not covered - decouples business logic from native error->proto error conversion - no need for original copy - field[x] is more idiomatic than field.get(x).unwrap() - don't need mutable access to fields
+ Use our read_file helper for better error context
- remove strings from ftl - remove color picker component - remove from cloze generation - remove icons for two buttons - use constant color for shapes
- rename mask to inactive shape and active shape color - border witdth and border color - change decimal point deserializing string and toFixed(2) - add thin border in mask editor, may be image background was transparent
- do not draw fixed ratio shapes by turn of uniformScaling - fix rectangle width,height - fix ellipse rx,ry,width,height - fix polygon postion and points - draw outside of canvas also
- rename variable
- move shapes at boundry when pointer is outside of canvas - include rx, ry for ellipse only - include points for polygon only
- fix shapes at edges
- implemented undo redo using fabric canvas events - polygon is special case and implemented only added and modified event - rectangle and ellipse have object:added, object:modified and object:removed case - change id to undo and redo
…ow canvas editor - set image width and height after adding image
- fix shapes at edges - toggle masks button to show/hide masks - hide clozes string, it contains <br> - set height for div container (used 'relative' in css)
- rename cursor tools - add left and right border
krmanik
force-pushed
the
feature-image-occlusion
branch
from
76f7d27
to
da2aad7
Compare
|
Thanks for reviewing.
When shapes are created an id are assigned but when shapes are added after removing using undo, the id are not assigned so the function returned false, and undo/redo was not working. But I have fixed the issues.
I have used transparent color in draw mode and in other mode default color. |
|
Thanks for all you work on this Mani, and for your input Aristotelis. It feels polished, and this PR has waited long enough, so I think it's time to merge. I'll start looking into the notetype changes I mentioned above in early April. |
|
I will start implementing the button to editor toolbar. |
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