fix select all and change ordinal in edit mode in io #3109

krmanik · 2024-04-01T11:21:19Z

The edit mode needs careful review, I have come up with the solution, but it may need improvement.
@abdnh pinging for review.

Closes #3109

dae · 2024-04-01T11:48:27Z

I gave this a quick test, and it seems to fix the problem for me in both edit mode and the add screen.

krmanik · 2024-04-01T11:52:30Z

Initial test works, but after adding some notes with group mode then again in edit mode, ungroup last shapes then group some other shapes. The ordinal seems to change and causing error something like No cloze found on number.

krmanik · 2024-04-01T12:45:39Z

The solution after changing group item can be fixed by introducing a new method which checks for ordinal number in cloze strings and reorder, the issue also fixed by Tools -> Empty Cards. The complicated solution may be reduced by just using just ordinal = 0, but some users may want to have same ordinal in edit mode for other shapes.

Edit:
Neither solution going to work, because before group total card in note type is less but when ungroup the cards increased but again grouping does not decrease total cards. The issue needs to be fix separately.

dae · 2024-04-02T08:13:59Z

Sorry Mani, could I trouble you to explain the issue in more detail? If a user reduces the number of occlusions, I'd expect that to break one or more of their cards. It would be nice if we could display a useful message during review in that case, like we do when there's an empty cloze. But I don't think we need to be cleaning up the cards until the user uses Tools>Empty Cards.

krmanik · 2024-04-02T09:14:09Z

In this demo, first I have created group shapes cloze, then ungroup it in edit mode, after that again group the shapes, it needs to remove empty cards.
We may show popup that after group and ungroup some cards have been updated and it needs to clear empty cards.

output.mp4

krmanik · 2024-04-03T01:43:25Z

The ordinal is undefined for all shapes when group/ungroup tools used, so that the cloze number should be in order.

dae · 2024-04-03T05:49:44Z

Hmm, this doesn't seem to work - if I add two rectangles to a new card, then edit the card and select all+group, the two shapes stay disconnected when the editor is closed.

krmanik · 2024-04-03T07:14:30Z

Now it will work as expected, but the users still have to use Tools -> Empty Cards.

I have used, to fix I have to use rm -rf out node_modules

PS C:\Users\mani\Documents\GitHub\anki> .\tools\ninja.bat format
....
PS C:\Users\mani\Documents\GitHub\anki> .\tools\ninja.bat check 
    Finished release [optimized] target(s) in 0.43s
[1/1; 1 active; 7.639s] build:configureninja: error: 'node_modules/@sveltejs/kit/src/types/synthetic/+dynamic+private.md', needed by 'out/tests/dprint.check', missing and no known rule to make it

dae · 2024-04-03T07:55:14Z

I've pushed a fix for the dprint error. Tests aren't currently passing, but seems to work well when I tried the grouping operation again with the latest code.

krmanik · 2024-04-03T08:17:31Z

I've pushed a fix for the dprint error. Tests aren't currently passing, but seems to work well when I tried the grouping operation again with the latest code.

Thanks, I have rebased it to main.

abdnh · 2024-04-03T08:23:51Z

ts/routes/image-occlusion/shapes/to-cloze.ts

@@ -166,7 +168,7 @@ function shapeOrShapesToCloze(

    // Maintain existing ordinal in editing mode
    let ordinal = shapeOrShapes.ordinal;
-    if (ordinal === undefined) {
+    if (ordinal === undefined || get(groupItemsModified)) {


This can break the ordinals of ungrouped masks when a grouping operation is done on other masks in edit mode.

Can we instead update the ordinals of grouped objects when they are grouped?
For example, If we group objects with the ordinals 2 and 4, all objects in the group will be assigned a single ordinal (maybe the smallest one, so 2).

This sounds like a good way of decreasing the number of empty cards we end up producing.

I don't think so, because let's say a user have created group cloze with more than two shapes as single group cloze, then during review he edited the shape to ungroup to individual shapes. Now total cards increased from one to total number of shapes. Now again during review he split it into two group, now ordinal must start from beginning to have consistent result, (Card 1 and Card 2)

I don't think we can solve this for all situations, especially if the user is closing and opening the editor between changes. I'd assumed that if we give new ordinals to shapes when they're removed from a group, that would be sufficient?

The solution in this PR should be accepted because if user modified the group items, there needs to implement to change ordinal for group items only, needs to keep track of group items, so instead of handling those cases, we should warn user that group and ungroup of shapes leads to changes of ordinal so refrain from doing this. But if no group shapes are touched the ordinal also does not modified.

I'd assumed that if we give new ordinals to shapes when they're removed from a group, that would be sufficient?

I am implementing and testing it.

Just to check we're on the same page - would you like this PR merged in without the changes that Abdo suggested?

I have thought of implementation idea for Abdo suggestion, I have implemented it in next commit, and it seems, it also solves the empty card issues. But it still needs some review.

Users need to do Tools -> Empty Cards, I have tried to solve but once cards added, the ordinal decreases but Cards remains same, so it needs to be fixed separately.

Sorry, I'm not quite following - what do you think needs to be done differently?

Nothing, I was implementing different solution when ungroup shapes. In case of ungroup shapes, find all the ordinal from shapes excluding groups shapes then start loop from beginning and put the ordinal to shapes in order. But I implemented simplest solution, use max ordinal (increment if required).

ts/routes/image-occlusion/tools/lib.ts

dae · 2024-04-04T09:00:45Z

ts/routes/image-occlusion/tools/lib.ts

@@ -63,12 +63,19 @@ export const groupShapes = (canvas: fabric.Canvas): void => {

    const activeObject = canvas.getActiveObject() as fabric.ActiveSelection;
    const items = activeObject.getObjects();
+
+    // @ts-expect-error not defined
+    const minOrdinal = Math.min(...items.map((item) => item.ordinal));


Something that might be nice in the future (not in this PR) is to tell TypeScript about the additional properties we've added to these objects, so we don't need to ignore errors/cast them to any each time we access them. I think it may be possible in a .d.ts file?

This can be done, I will look into this.

dae · 2024-04-04T09:08:26Z

Thanks Mani, this seems to work well. @abdnh, any thoughts/comments? If not, I'll merge this in tomorrow and get a 24.04.1rc out.

abdnh · 2024-04-04T18:39:23Z

ts/routes/image-occlusion/tools/lib.ts

+
+    items.forEach((item, index) => {
+        // @ts-expect-error not defined
+        item.set({ opacity: get(opacityStateStore) ? 0.4 : 1, ordinal: maxOrdinal + index + 1 });


This is not working as expected. It can result in two unrelated masks getting the same ordinal, causing them to be grouped. To reproduce:

Add 3 masks

Group two of the masks

Ungroup

Reload the note (e.g. by selecting another browser note then back to the note)

Notice one mask from the original group is grouped with the third.

I have updated the implementation, it should work now, but order cannot be maintained in case of ungrouping of shapes.

Thanks for catching that Abdo! And thank you Mani for your continued work on this. I gave it a quick try again and couldn't seem to trip it up. When you say the order can't be maintained, I wonder if that's a problem? As long as we're not reusing ordinals, what problems do you think it would cause?

No problem, we can go with this.

@dae While the issue I reported is fixed, this is re-introducing the original problem fixed by #2987 when there's an ungroup operation. Unrelated masks could get reordered.

This appears to work better:

diff --git a/ts/routes/image-occlusion/shapes/to-cloze.ts b/ts/routes/image-occlusion/shapes/to-cloze.ts index 01d8b62c9..fbdd03f07 100644 --- a/ts/routes/image-occlusion/shapes/to-cloze.ts +++ b/ts/routes/image-occlusion/shapes/to-cloze.ts @@ -3,9 +3,7 @@ import { fabric } from "fabric"; import { cloneDeep } from "lodash-es"; -import { get } from "svelte/store"; -import { shapeUngroupState } from "../store"; import { getBoundingBox } from "../tools/lib"; import type { Size } from "../types"; import type { Shape, ShapeOrShapes } from "./base"; @@ -22,6 +20,7 @@ export function exportShapesToClozeDeletions(occludeInactive: boolean): { let clozes = ""; let index = 0; + let nextOrdinal = 1; shapes.forEach((shapeOrShapes) => { // shapes with width or height less than 5 are not valid if (shapeOrShapes === null) { @@ -31,7 +30,33 @@ export function exportShapesToClozeDeletions(occludeInactive: boolean): { if (shapeOrShapes instanceof Rectangle && shapeOrShapes.fill === "transparent") { return; } - clozes += shapeOrShapesToCloze(shapeOrShapes, index, occludeInactive); + // Maintain existing ordinal in editing mode + let ordinal: number | undefined; + if (Array.isArray(shapeOrShapes)) { + ordinal = shapeOrShapes[0].ordinal; + } else { + ordinal = shapeOrShapes.ordinal; + } + if (ordinal === undefined) { + if (shapeOrShapes instanceof Text) { + ordinal = 0; + } else { + ordinal = nextOrdinal; + } + if (Array.isArray(shapeOrShapes)) { + shapeOrShapes.forEach((shape) => (shape.ordinal = ordinal)); + } else { + shapeOrShapes.ordinal = ordinal; + } + } + nextOrdinal = ordinal + 1; + + clozes += shapeOrShapesToCloze( + shapeOrShapes, + index, + ordinal, + occludeInactive, + ); if (!(shapeOrShapes instanceof Text)) { index++; } @@ -134,6 +159,7 @@ function fabricObjectToBaseShapeOrShapes( function shapeOrShapesToCloze( shapeOrShapes: ShapeOrShapes, index: number, + ordinal: number, occludeInactive: boolean, ): string { let text = ""; @@ -145,7 +171,7 @@ function shapeOrShapesToCloze( let type: string; if (Array.isArray(shapeOrShapes)) { return shapeOrShapes - .map((shape) => shapeOrShapesToCloze(shape, index, occludeInactive)) + .map((shape) => shapeOrShapesToCloze(shape, index, ordinal, occludeInactive)) .join(""); } else if (shapeOrShapes instanceof Rectangle) { type = "rect"; @@ -166,16 +192,6 @@ function shapeOrShapesToCloze( addKeyValue("oi", "1"); } - // Maintain existing ordinal in editing mode - let ordinal = shapeOrShapes.ordinal; - if (ordinal === undefined || Number.isNaN(ordinal) || get(shapeUngroupState)) { - if (type === "text") { - ordinal = 0; - } else { - ordinal = index + 1; - } - shapeOrShapes.ordinal = ordinal; - } text = `{{c${ordinal}::image-occlusion:${type}${text}}}<br>`; return text; diff --git a/ts/routes/image-occlusion/store.ts b/ts/routes/image-occlusion/store.ts index b1653f4fa..5847250dd 100644 --- a/ts/routes/image-occlusion/store.ts +++ b/ts/routes/image-occlusion/store.ts @@ -17,5 +17,3 @@ export const ioImageLoadedStore = writable(false); export const opacityStateStore = writable(false); // store state of text editing export const textEditingState = writable(false); -// store state of shape ungroup -export const shapeUngroupState = writable(false); diff --git a/ts/routes/image-occlusion/tools/lib.ts b/ts/routes/image-occlusion/tools/lib.ts index 9a6738b10..bc0606cdc 100644 --- a/ts/routes/image-occlusion/tools/lib.ts +++ b/ts/routes/image-occlusion/tools/lib.ts @@ -4,7 +4,7 @@ import { fabric } from "fabric"; import { get } from "svelte/store"; -import { opacityStateStore, shapeUngroupState } from "../store"; +import { opacityStateStore } from "../store"; export const SHAPE_MASK_COLOR = "#ffeba2"; export const BORDER_COLOR = "#212121"; @@ -94,11 +94,14 @@ export const unGroupShapes = (canvas: fabric.Canvas): void => { canvas.remove(group); items.forEach((item) => { - item.set({ opacity: get(opacityStateStore) ? 0.4 : 1 }); + item.set({ + opacity: get(opacityStateStore) ? 0.4 : 1, + // @ts-expect-error not defined + ordinal: undefined, + }); canvas.add(item); }); - shapeUngroupState.set(true); redraw(canvas); };

What do you think @dae @krmanik ?

Edit:

It needs to check Number.isNaN(ordinal), working now. Thanks Abdo.

Nice catch @abdnh! And thanks for the quick fix @krmanik.

Unfortunately I ran into another issue. I created three rectangles and merged them into a group, then created three more, and left them ungrouped. I added the note. Then I edited it, and ungrouped the three rectangles. Two surprises:

The group's ordinal was not reused, so the first card is now empty and needs to be cleaned up with tools>empty cards

One of the shapes in the group was given the same ordinal as one of the shapes that was never in a group, so it ended up in a new group.

krmanik · 2024-04-05T03:15:32Z

I was trying to implement this, but it works sometimes and sometimes does not, but the simplest solution will be just use svelte store only for ungroup shapes which works now.

Code

diff --git a/ts/routes/image-occlusion/tools/lib.ts b/ts/routes/image-occlusion/tools/lib.ts
index f7d00846c..38fccd703 100644
--- a/ts/routes/image-occlusion/tools/lib.ts
+++ b/ts/routes/image-occlusion/tools/lib.ts
@@ -93,18 +93,53 @@ export const unGroupShapes = (canvas: fabric.Canvas): void => {
     group.destroyed = true;
     canvas.remove(group);
 
-    // @ts-expect-error not defined
-    const maxOrdinal = Math.max(canvas.getObjects().map((item) => item.ordinal));
+    // take ordinals for all shape excluding the ones in the group
+    const excludedObjectOrdinals = getCanvasObjects(canvas)
+        .filter((obj) => !items.includes(obj))
+        .map((item) => item.ordinal)
+        .filter((ordinal) => ordinal !== undefined || !Number.isNaN(ordinal));
+
+    console.log("excludedObjectOrdinals", excludedObjectOrdinals);
+
+    const totalShapes = getCanvasObjects(canvas).length;
+
+    console.log("totalShapes", totalShapes);
+
+    const newOrdinal = [];
+    for (let i = 1; i < totalShapes; i++) {
+        if (!excludedObjectOrdinals.includes(i)) {
+            newOrdinal.push(i);
+        }
+    }
+
+    console.log("newOrdinal", newOrdinal);
 
     items.forEach((item, index) => {
         // @ts-expect-error not defined
-        item.set({ opacity: get(opacityStateStore) ? 0.4 : 1, ordinal: maxOrdinal + index + 1 });
+        item.set({ opacity: get(opacityStateStore) ? 0.4 : 1, ordinal: newOrdinal[index] });
         canvas.add(item);
     });
 
     redraw(canvas);
 };
 
+const getCanvasObjects = (canvas) => {
+    const objectList = [];
+    canvas.forEachObject(function(object) {
+        if (object.type === 'group') {
+            const group = object;
+            group.forEachObject(function(obj) {
+                objectList.push(obj);
+            });
+        } else {
+            objectList.push(object);
+        }
+    });
+
+    // filter out the transparent bounding box from the selection
+    return objectList.filter((obj) => obj.fill !== "transparent");
+}
+
 const copyItem = (canvas: fabric.Canvas): void => {
     const activeObject = canvas.getActiveObject();
     if (!activeObject) {

dae · 2024-04-05T12:26:51Z

@abdnh any other thoughts, or happy for this to be merged in?

krmanik · 2024-04-09T04:14:57Z

ts/routes/image-occlusion/shapes/to-cloze.ts

@@ -21,6 +21,9 @@ export function exportShapesToClozeDeletions(occludeInactive: boolean): {
    let clozes = "";
    let index = 0;
    let nextOrdinal = 1;
+
+    console.log(shapes);


Adding this console log seems to fix the issue, so is the issue race condition? Then may be we should use promise and resolve or settimeout. (It works when Ankiweb inspect opened)

I'm afraid for me, the same issue still happens when I update to the latest commit which has this console.log call. I tried 3 times, and it happened on the 1st and 2nd time, but not the 3rd. If you try a few more times, does it ever happen? Could it be something other than a race condition, such as unpredictable dictionary ordering?

I didn't understand the underlying issues, so I have changed the implementation, it works as expected. The general idea is takeout ordinal which does not assign to any shape from a list of ordinal and assign to shape.

…ed, in add mode no ordinal preset so NaN

krmanik · 2024-04-10T10:33:50Z

The order of shapes in group issue is resolved now in latest commit.

dae · 2024-04-10T10:42:57Z

Great, thanks Mani. Will review this tomorrow. Abdo, if you have a chance before then, your input would be appreciated.

…

On Wed, 10 Apr 2024 at 5:34 pm, Mani ***@***.***> wrote: The issue is resolved now in latest commit. — Reply to this email directly, view it on GitHub <#3109 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABMCPRKYCPV5NO5AROFRIDY4UISJAVCNFSM6AAAAABFROZJ32VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBXGE3TANJZGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

dae · 2024-04-11T06:51:25Z

Thanks Mani, this seems to work well, and if there are any issues with the code, I didn't notice them. ^_^; I'm going to merge this in now so I can get a beta out today, but if you have any concerns @abdnh, please let us know.

* fix select all and change ordinal in edit mode in io * make ordinal undefined for all shapes in group/ungroup * fix group shapes and some ui fixes * Don't add node_modules/* to dprint deps * use minimum ordinal when shape merged, use max ordinal++ when ungrouped, in add mode no ordinal preset so NaN * use state for ungroup shape * maintain existing ordinal in editing mode * fix order of ordinal in ungroup shape * refactor: remove for loop, use forEach (cherry picked from commit 477f932)

abdnh · 2024-04-11T15:56:29Z

Looking good to me. Didn't notice any issues either.

krmanik force-pushed the fix-io-group-tool branch from dc0eb14 to 49d6c38 Compare April 1, 2024 12:33

krmanik force-pushed the fix-io-group-tool branch 2 times, most recently from 5ea6334 to 49cd788 Compare April 3, 2024 08:15

abdnh reviewed Apr 3, 2024

View reviewed changes

krmanik commented Apr 3, 2024

View reviewed changes

ts/routes/image-occlusion/tools/lib.ts Outdated Show resolved Hide resolved

krmanik force-pushed the fix-io-group-tool branch 2 times, most recently from d330ed2 to 05bd940 Compare April 3, 2024 14:06

dae reviewed Apr 4, 2024

View reviewed changes

abdnh reviewed Apr 4, 2024

View reviewed changes

krmanik force-pushed the fix-io-group-tool branch from 05bd940 to 2511c57 Compare April 5, 2024 03:15

krmanik force-pushed the fix-io-group-tool branch from 2511c57 to d134dde Compare April 7, 2024 02:54

krmanik commented Apr 9, 2024

View reviewed changes

dae mentioned this pull request Apr 10, 2024

"Select All" does not work properly in image occlusion in 24.04 #3107

Closed

krmanik and others added 4 commits April 10, 2024 18:21

fix select all and change ordinal in edit mode in io

0b63127

make ordinal undefined for all shapes in group/ungroup

979fc13

fix group shapes and some ui fixes

8314f39

Don't add node_modules/* to dprint deps

59379cd

krmanik added 4 commits April 10, 2024 18:21

use minimum ordinal when shape merged, use max ordinal++ when ungroup…

dba075f

…ed, in add mode no ordinal preset so NaN

use state for ungroup shape

7710851

maintain existing ordinal in editing mode

77ec273

fix order of ordinal in ungroup shape

458c9e1

krmanik force-pushed the fix-io-group-tool branch from cf431a7 to 458c9e1 Compare April 10, 2024 10:22

refactor: remove for loop, use forEach

abef36a

dae merged commit 477f932 into ankitects:main Apr 11, 2024
1 check passed

krmanik deleted the fix-io-group-tool branch April 12, 2024 02:04

abdnh mentioned this pull request Apr 12, 2024

Improve typing of custom fabric.Object properties #3134

Merged

fix select all and change ordinal in edit mode in io #3109

fix select all and change ordinal in edit mode in io #3109

Conversation

krmanik commented Apr 1, 2024 • edited by dae Loading

dae commented Apr 1, 2024

krmanik commented Apr 1, 2024

krmanik commented Apr 1, 2024 • edited Loading

dae commented Apr 2, 2024

krmanik commented Apr 2, 2024

krmanik commented Apr 3, 2024

dae commented Apr 3, 2024

krmanik commented Apr 3, 2024 • edited Loading

dae commented Apr 3, 2024 • edited Loading

krmanik commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmanik Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmanik Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dae commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmanik Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmanik commented Apr 5, 2024 • edited Loading

dae commented Apr 5, 2024

krmanik Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmanik commented Apr 10, 2024 • edited Loading

dae commented Apr 10, 2024 via email

dae commented Apr 11, 2024

abdnh commented Apr 11, 2024

krmanik commented Apr 1, 2024 •

edited by dae

Loading

krmanik commented Apr 1, 2024 •

edited

Loading

krmanik commented Apr 3, 2024 •

edited

Loading

dae commented Apr 3, 2024 •

edited

Loading

krmanik Apr 3, 2024 •

edited

Loading

krmanik Apr 3, 2024 •

edited

Loading

krmanik Apr 7, 2024 •

edited

Loading

krmanik commented Apr 5, 2024 •

edited

Loading

krmanik Apr 9, 2024 •

edited

Loading

krmanik commented Apr 10, 2024 •

edited

Loading