PhonologicalCorpusTools / SLPAA

5 stars 0 forks source link

Unlocking orientation module #244

Closed brdiep113 closed 9 months ago

brdiep113 commented 1 year ago

Issue #75 Orientation module unlocked, still confirming that module options can be appropriately loaded and saved.

brdiep113 commented 11 months ago

The functionality for saving should be complete now! Last thing before I finish this is to make sure that the error messages are correct for the cases we discussed. I lost the note I left for it so if @kchall you could make sure it's working as intended that'd be great!

kchall commented 11 months ago

Okay, @brdiep113 (and @kvesik so you know not to worry about checking this yet):

There seem to be a few issues, both with the error messages as you surmised and also with the actual functionality.

Error message issues:

  1. I had originally said (https://docs.google.com/document/d/15KPfPauWFHdlA2LDuNKVOsYFT9enws9HvWTNzIhu04I/edit#bookmark=id.kk4n4i91ax7g) that the error messages / expectation that there are both palm direction and finger root specification can be over-written. But I think it's probably just easier if we take out that requirement, and allow a user to save an 'empty' orientation module with just the hand and timing specified. We allow this with all other modules, so even though it's not actually expected behaviour, I think it's easier than trying to get the specific combinations right. So, we can remove all checks for palm and finger root direction and all error messages associated with them.
  2. That's good, because currently if a user tries to save an empty module, the error message says that they must specify finger root and palm direction, but actually only enforces palm direction. This is a moot point if we just get rid of all of these as in (1).
  3. Currently, if only a checkbox is selected for palm direction (and not a radio button underneath it), this also triggers an error message, as though palm direction hadn't been specified. Again, this is probably a moot point if we get rid of all checking, but this should not have been triggering the error message in the first place.

Functionality issues:

  1. If I create a new orientation module and input a radio button option for both palm and finger root and save and close the module, and then re-open the module, at first everything looks good. But if I then uncheck one of the orientation options, or change one of the options, or even add an additional option, then save, and re-open, it has reverted to the original version -- none of the new elements have been saved. This is true even if I "restore defaults" and set up a whole new set of options -- when I save and re-open, the very first original options are shown. (Note: changes to timing and hand selection are saving normally.)
kchall commented 10 months ago

@brdiep113 Thanks for continuing to work on this! It looks like currently, the error message issues are resolved (thanks!), but the saving functionality issue is still a problem. Are you still working on this, or would you like to just be done with it and have me re-assign it?

brdiep113 commented 10 months ago

@brdiep113 Thanks for continuing to work on this! It looks like currently, the error message issues are resolved (thanks!), but the saving functionality issue is still a problem. Are you still working on this, or would you like to just be done with it and have me re-assign it?

Still working on this! I'm tracking down the specifics of module saving since it seems like saving into an existing module is not working.

kvesik commented 10 months ago

@brdiep113 see Sign.updatemodule()

brdiep113 commented 10 months ago

Sorry for the really long delay @kchall @kvesik I think I got the saving to work correctly now! Let me know if the functionality works as expected!

brdiep113 commented 10 months ago

Also just upon resolving this issue, not sure if this should be a new issue but I think we can rewrite Sign.updatemodule() to be much cleaner and work for all the modules.

Previously image

Rewritten to be more general and function the same for all module types. Didn't do much testing so I don't know if this will work for all our modules but from my first look it looks like all we do is confirm for all attributes of the module type, that we overwrite the current with the updated if they differ: image

Which should prevent us from having to do moduletype checks and make this extendable in the future.

kchall commented 10 months ago

@brdiep113 Thanks, this looks to be working correctly now as far as I can tell! Yay!

I will let @kvesik weigh in on the proposed revision to Sign.updatemodule().

In the meantime -- thank you so much for your work on the project!