Add dependency between "sign type" and individual modules

[kchall]

If "sign type" is set to be "1 hand," then the movement, location, hand configuration, and orientation modules should open with "Hand 1" selected by default, but both "Hand 2" and "Both hands" still available for selection. If the user tries to save the module with "Both hands" selected, a warning message should be given: "The sign type for this sign is 1-handed. Are you sure this module should apply to both hands?" ...with button choices: "Return to edit this module" / "Continue with both hands."

Note that this really does apply only to the "hand" options in these modules -- e.g. a user could still specify an arm or leg movement or location.

The relation module doesn't need to have anything pre-specified; while there are some likely logical options, it's not worth the extra complications.

If "sign type" is set to be "2 hands," nothing should happen, because any individual module could apply to one or both hands.

[kvesik]

We called this a "good second issue" on June 13 2024. Should be a pretty straightforward fix once you understand the underlying data structures.

[kvesik]

Good place to start: ArticulatorSelectionPanel, used in ModuleSelectorDialog.

Also the ModuleSelectorDialog.validate_and_save() function.

[Harukaichii]

@kchall Just to confirm, nothing should be done for the non-manual module regardless of what the user selects?

@Harukaichii That's correct!

[Harukaichii]

331-q1.mov

Adding on: does this mean the "Hand", "Arm", and "Leg" radio button should each have their own state?

currently it looks like if I press "2 hands" then it automatically assumes "2 arms" and "2 legs" (and I can't change one without changing everything else).

So I understand only 1 of them will be selected but if something is pre-populated for the hands, should the state reset to no selection if arms or legs are clicked?

@Harukaichii Hmm, interesting; I hadn't noticed that linked behaviour before. Just to be clear, any instance of the module should actually only apply to one of the hands/arms/legs options, not more than once simultaneously (which is different than the Sign Type module itself, where the user can specify [or will be able to specify, pending #332 ] hands, arms, AND legs if they want). I think we should probably de-link the behaviour within a specific module, though, such that if the user clicks on 'both hands,' nothing is selected for the 'arms' and 'legs' options. But, we should then also extend the logic of this original issue to apply to the Sign Type updates in #332 . That is, if a user has specified that the sign type is "1 leg" after #332 is implemented, then if they toggle to "Legs" in e.g. a Movement module, it should default to "Leg 1" as the selected leg, with "Leg 2" and "Both legs" available for selection. Does this make sense?

[Harukaichii]

Ok after poking around a bit more it seems that "Sign Type" can be configured per sign but I remembered we also have this "default sign type" option in preferences. It seems that this does set the value of "Sign Type" but you still have to manually create "Sign Type" for it to show up on the timeline.

Is this expected behaviour?

@Harukaichii Yes, this is expected (and desired). The "default sign type" just specifies what sign type will be used by default if the "Sign Type" dialog is initialized, rather than creating an actual instance of the dialog itself.

[kvesik]

@kchall just FYI when I originally implemented the different types of articulators (hand vs arm vs leg), it seemed that the most efficient way to do so was to store three pieces of info: (1) whether the module applies to articular 1, (2) whether the module applies to articular 2, and (3) what kind of articulators are being referenced by this module (ie hand vs arm vs leg).

So @Harukaichii if that is no longer a reasonable or efficient assumption, do please feel free to separate articulators into three separate underlying data structures, one for each of 2 hands / 2 arms / 2 legs. Might be a bit of funky backward compatibility stuff to figure out, but shouldn't be too crazy.

[Harukaichii]

@kvesik cool thanks so much for the clarification. I'll look more into it and keep you updated on what I find :)

[kchall]

@Harukaichii This basically looks great! But if I have selected "2 hands" in "Sign Type," it seems to be defaulting to selecting "Both hands" in the various modules. This is incorrect; according to the way I originally stated the issue, nothing should be selected:

"If "sign type" is set to be "2 hands," nothing should happen, because any individual module could apply to one or both hands."

This still is basically true, so I'd like that functionality to be updated.

That said, one exception to this would be if "Sign Type" is specified as 2 Hands and then there is also information given about the specific movement characteristics. The complete list of options would be as follows:

Note that I do recognize that there might be some other combinations that could be pre-set, around the hand configurations, the "both hands move" options, and the symmetry options, but the logic on these gets pretty complicated, so I don't think it's worth trying to program them. We'll just have to trust users to do things correctly at least for now. :)

[Harukaichii]

Hi @kchall
Thanks so much for pointing that out. I gotta work on my description reading skills haha.

So for now I addressed the

"If "sign type" is set to be "2 hands," nothing should happen, because any individual module could apply to one or both hands."

part.

So just to clarify, you also would you like me to look into implementing customized warning texts if it's a movement module and 2 hands are selected?

[kchall]

Thanks, @Harukaichii -- appreciate the update.

I tried to double-check that the current branch matches the original request, but I am getting an error message when trying to sync to the 331 branch:

(Note that I don't personally have any changes on main.py that would be being uploaded.)

Re: the last question, yes, if you wouldn't mind expanding this work to cover the movement module dependencies, that would be great. Thanks!

[Harukaichii]

That is really bizarre!
Since on the pull request it says there should be no conflicts with the main branch

Do you have the latest version of main downloaded?

If not, let me do the other part you requested and then perhaps we can do a zoom troubleshoot tomorrow or during Thursdays meeting?

[kchall]

@Harukaichii Yep, I'm up-to-date on main. We can discuss at Thursday's meeting!

[kchall]

^ @Harukaichii Thanks for pointing out the typo in this line! The error message in this case should say "The sign type for this sign specifies that only one hand moves. Are you sure you want this movement module to exist?"

I'll double check the functionality and wording on the save buttons shortly. :)

[Harukaichii]

@kchall I updated the warning message.

Feel free to check the functionality whenever :).

[kchall]

@Harukaichii Thank you!

Okay, I think I must have somehow posted this whole issue before actually being finished with that table, because I've realized a couple of other things that I meant to do slightly differently (to be clear: not your fault! and I apologize for being sloppy). That said, in testing what we currently have, it does seem like there are a couple of small errors as well. Let me see if I can say all this clearly this time.

1. I'm totally happy with your change in the wording to the button where it now says "Continue with saving module." I've changed the text to be green in the first table below; you've already implemented it, and it can continue this way.
2. I forgot to include the case where the Sign Type is one hand, and the hand doesn't move. This is now added in the first table below as the top row, in blue. It would be great if you could add this.
3. I gave you the incorrect wording on the three cases where it's a two-handed sign and only one hand moves. The warning message should reference the mismatch between the sign type and the way the movement module has been set up, rather than asking about the existence of the movement module. I've corrected the wording in red in the first table below.

Now, in terms of functionality: it's definitely possible that some of the incorrect implementation is due to my incorrect wording originally, which might have given you the wrong idea about my intentions (my apologies!). But I also think that there's at least a few other actual errors in the implementation.

In the tables below, I've tried to go through every combination to test it. I'm checking for three things: first, what happens if I try to save a movement module that is correctly set up? (does it allow me to proceed with no warning?) second, what happens if I try to save a movement module that is incorrectly set up? (do I get the right warning message?) and third, if I go back to edit the module and fix it, does the warning message disappear? For the first of these checks, there are a few places where even if the module is set up correctly, I get a warning message. These are highlighted in green below. For the second, there are again a few places where the software is just incorrect; it's giving me a warning message about the wrong hand. These are highlighted in light blue in the table below. For the third check, there are a few places where even when the error is fixed, the software still throws the warning (probably because it throws the warning even if it was done correctly in the first place!); these are highlighted in yellow below.

Let me know if any of this isn't clear -- thank you!

[Harukaichii]

@kchall
Alright, you got me too-- thought I could resolve it before the weekend but looks like it was more complex than expected. (also had a typo with a variable name which is why you were getting some unexpected behaviour haha)

I updated the code and I sat down and documented all the test cases (using yours as a starting point)

https://docs.google.com/document/d/1_FPB1UwQdUZJ-72K6rgn8E0e3AkdXR3txyZqAFWTjyc/edit?usp=sharing

Here's the document. Feel free to look over it. I highlighted the messages I'd like a double check on.

If I didn't write (none for all) and the box has just H1, H2 ,both hands, that means I skipped the test case because based on my code, it's technically already covered by one of the other cases.

I didn't document the button text because it's just fixed for all the cases.

Hopefully it's better now 🤞

[kchall]

@Harukaichii Awesome! I think this is all basically correct now. There's just one place where I think you've extended the functionality from what I originally requested, and that's basically fine! -- but it's now missing one component. Specifically, if the sign type is "1 hand" then in any non-movement module, it's fine to pre-populate the module with Hand 1 (though I don't think I specifically asked for this). Currently, you've done this for all non-movement modules only if the sign type if "1 hand" or "1 hand, the hand moves," and not done it for "1 hand, the hand doesn't move." That is what should happen for the movement modules, but for the non-movement modules, the fact that the hand doesn't move shouldn't matter. I've also marked this on your table. But otherwise, I've checked everything and it all seems to be working, yay!

Also, in case it's useful (though I recognize it's a bit late, ha), here's a mini-corpus I made with all the different sign types, for easier testing purposes: https://www.dropbox.com/scl/fi/2dzkgk11v7annyruyjhm9/issue331_test_corpus.slpaa?rlkey=fknm2pg0szw82fhuljcl83j4s&dl=0

[Harukaichii]

@kchall
Thank you so much for all your patience with testing and verifying :).
That makes a lot of sense to me, and I pushed this change: ee9458d

which only affects the condition of when we pre-set values for conditions under one hand

The condition now says, pre-set values if:

• sign type is one handed AND it is NOT a movement module
  OR
• sign type is one hand AND it is NOT one handed non-movement setting AND it is a movement module

I followed up by testing the following 6 cases:

• non-movement module
  --- sign type: 1hand
  --- sign type: 1hand hand moves
  --- sign type: 1hand no movement

• movement-module
  ---sign type: 1hand
  --- sign type: 1hand hand moves
  --- sign type: 1hand no movement

Let me know if you have further feedback!

[kchall]

@Harukaichii Excellent, this all seems to be working as expected! :)

[Harukaichii]

@kchall
Fantaaastic!

I will wait for @kvesik to do a code check sign off and then I'll merge

[Harukaichii]

Resolved with #368