PhonologicalCorpusTools / SLPAA

5 stars 0 forks source link

169: Add phonetic phonological options all #350

Closed Harukaichii closed 3 months ago

Harukaichii commented 3 months ago

Addresses: #169

Description:

Demo: 169

Harukaichii commented 3 months ago

FYI, from my current version of main, I'm seeing this error when i try to save for hand configuraiton:

スクリーンショット 2024-07-15 午後9 33 20
kchall commented 3 months ago

@Harukaichii Thanks so much! I'll let @kvesik comment on the implementation, but one note about the user-side: note that #169 specifies that the "major/minor" choices are only for the location module; those aren't relevant options for the other modules, where it's just a choice between "phonological" and "phonetic." I'm sure that makes it slightly more complicated -- my apologies!

kvesik commented 3 months ago

@Harukaichii I have to prioritize other work today but will follow up on SLPAA stuff (including this) tomorrow.

Harukaichii commented 3 months ago

@Harukaichii Thanks so much! I'll let @kvesik comment on the implementation, but one note about the user-side: note that #169 specifies that the "major/minor" choices are only for the location module; those aren't relevant options for the other modules, where it's just a choice between "phonological" and "phonetic." I'm sure that makes it slightly more complicated -- my apologies!

@kchall My bad haha! I totally skipped that part of the description 🤦‍♀️ I have some follow up questions though: 1) Is it possible to have other sub-options for different modules? 2) Will the user ever be able to create their own module? (or create some custom module?)

kchall commented 3 months ago

@Harukaichii Good questions!

  1. Is it possible to have other sub-options for different modules? No, I don't think so. The only module I can think of that might have a similar distinction would be the movement module, where people do distinguish between 'path' and 'internal' movement. But we already distinguish these within the movement module, as 'perceptual shape' and 'joint-specific,' so I don't think we need to have another specification of this.

  2. Will the user ever be able to create their own module? (or create some custom module?) Probably not. We originally did have some ideas about making the software more customizable, but practical considerations have mostly taken that off the table. While I'm in general in favour of making choices that would support such flexibility in the future, it's also more important to me that we have the options we know we need than that we theoretically support options that we don't have any immediate ideas for implementing.

Harukaichii commented 3 months ago

@kchall Got it, thanks for the clarifications. I'll see what @kvesik has to say about my current implementation and I'll update the PR tomorrow.

And @kvesik after reviewing everything, what do you say to keep the underlying PhonLocations structure the same and just have a hard coded check on the UI to only show the suboptions if the module is Location (the more general approach would be to allow each module to bring their own set of suboptions but I don't think that's necessary after the answer to 1)

But yea, it also highly depends on how you want to handle the data migration if we change PhonLocations. Since, having two properties that are only used for Location being saved everytime is not ideal but I also don't know what your priorities are for optimizing space right now haha.

kvesik commented 3 months ago

@Harukaichii I think the smoothest approach at this point is to leave the structure of PhonLocations the same, and just do the UI check as you suggested. Thanks!

Harukaichii commented 3 months ago

Newly updated windows:

スクリーンショット 2024-07-21 午後4 29 20 スクリーンショット 2024-07-21 午後4 30 34 スクリーンショット 2024-07-21 午後4 30 42 スクリーンショット 2024-07-21 午後4 30 52
Harukaichii commented 3 months ago

@kvesik I found a couple more things after reviewing. Can you also do me a favour and test the changes while trying to open a corpus in the old format that has a phonological or phonetic box checked for location?

Want to make sure my changes doesn't accidentally delete previous data. (i.e. idk if its actually backwards compatible haha)

kchall commented 3 months ago

@Harukaichii @kvesik Fortunately, we haven't been using the 'phonetic' and 'phonological' options in the location modules yet, so there's no need to worry about that particular piece of backward compatibility!

kvesik commented 3 months ago

@Harukaichii regarding backward compatibility, remember that you can always create a tiny test corpus using the main branch, and then reopen using your development branch. :)

But good to know that in this case it's not necessary anyway!