Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

331: Add warning before saving "both hands" for 1 handed signs #368

Merged
merged 10 commits into from
Sep 10, 2024

Conversation

Harukaichii
Copy link
Contributor

  • Added warning dialog when we encounter scenario described in title
  • deleted includephrase reference -- doesn't seem like it is being used?
Part1.mov
Part2.mov

Note: If you toggle between the options it will reset

2024-08-09.8.03.57.mov

@@ -302,18 +325,18 @@ def validate_and_save(self, addanother=False, closedialog=False):
modulevalid, modulemessage = self.module_widget.validity_check()

messagestring = ""
if not (articulatorsvalid and timingvalid):
if not articulatorsvalid:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so this was because I originally thought I needed to single out articulatorsvalid...

However, I think what I have now is easier to read as it has less conditionals & I've tested it still gives the same error messages.
But if we prefer the old way, I'm completely fine to change it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me :)


# validate hand selection
articulatorsvalid, articulators = self.validate_articulators()
# if the default is "1h" and we have both hands selected...
if articulators[0] == HAND and self.defaultHandArticulator == "1h" and articulators[1][1] and articulators[1][2]:
msgBox = QMessageBox()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if there's a cleaner way to make a QMessageBox with custom buttons :D

if HAND in incl_articulators and self.parent().sign.signtype:
# set default articulators
handSelection = self.parent().sign.signtype.specslist[0][0]
if "1h" in handSelection:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're ok with this logic, I thought of making 1h and 2h a constant string in the constants file. Not sure if that would be helpful elsewhere though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and we're not doing an exact match bc some of the options have more to the string than just 1h (which should be irrelevant for my use case)

@Harukaichii
Copy link
Contributor Author

Side note: going through the code is a bit of a pain due to the clustering of UI logic with business logic.

If I'm still here after Beta release, can I try out some ways to separate the two?

@Harukaichii Harukaichii requested review from kchall and kvesik August 15, 2024 17:57
@Harukaichii Harukaichii force-pushed the 331-add-dependency-btwn-signtype-ind-mod branch from e74b791 to db26c36 Compare August 20, 2024 20:14
Copy link
Collaborator

@kvesik kvesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, Queenie! Your changes all look fine to me. Let me know if you still want to address some of your specific comments, but I don't have any outstanding concerns.

@Harukaichii Harukaichii merged commit e085d65 into main Sep 10, 2024
kvesik added a commit that referenced this pull request Sep 24, 2024
…-associated-relation-modules

finish refactoring named arguments modified in PR #368
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants