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

Replace boolean gui.IsInMessageBox with function gui.message.isModalMessageBoxActive #13376

Merged
merged 7 commits into from
Mar 3, 2022

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Feb 22, 2022

Link to issue number:

Relates to #13007, as the new design should be backwards compatible, we can implement the API changes as-is for 2022.1.
Follow up to the API proposed in #12984

Summary of the issue:

The API for gui.message had not yet been determined for 2022.1, as per #13007.
As the future API is intended to be backwards compatible, this PR commits to the API proposed in #12984.

Description of how this pull request fixes the issue:

gui.IsInMessageBox was a boolean, this has been replaced by a function gui.message.isModalMessageBoxActive, which tracks if a messageBox is open in a safer manner.

Changes logic gui/message.py to be safer with locking and handling errors.

Testing strategy:

Alpha/beta testing of messageBoxes

Known issues with pull request:

None

Change log entries:

See PR diff

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@seanbudd seanbudd requested a review from a team as a code owner February 22, 2022 06:59
@seanbudd seanbudd added this to the 2022.1 milestone Feb 24, 2022
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

if isInMessageBox is still used as a bool variable, it will always be true. I think a new name should be used, so that that code that doesn't get updated throws an error, rather than doing the wrong thing. Consider:

def isInMessageBox():
	return False
print(bool(isInMessageBox))  # prints True

# contrived example of addon code that has not been updated.
if isInMessageBox:  # always true, instead it should error "name 'isInMessageBox' is not defined"
    log.debug("in the message box")

user_docs/en/changes.t2t Outdated Show resolved Hide resolved
@feerrenrut
Copy link
Contributor

Could you update this PR to make the title more descriptive of the change, and describe the changes that are part of this PR. Issue #12984 contains lots of suggestions, what does this PR do?

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Don't forget to update the API plan.

Thinking about what is important to point out in the plan:

  • This function can only tell you if 1 or more message boxes are active.
  • If, in the future, there can be multiple (non-modal) message boxes active, the API will need to be extended, perhaps with "getActiveMesageBoxes()". This is fine.

This second point makes me think this method should be specific to modal message boxes, I.E. rename to isModalMessageBoxActive(). With docs explicitly stating "only one modal message box may be active at once"

What do you think?

user_docs/en/changes.t2t Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member Author

With docs explicitly stating "only one modal message box may be active at once"

I don't believe this is true, as per #12984 (comment).

I think the name isModalMessageBoxBlocking might even be better. In the future API we might want to ignore non-blocking message boxes.

@feerrenrut
Copy link
Contributor

Yes, I'd forgotten that case, thought it is arguable that even in that case only one is active at a time. I.E. only one of them is processing messages.

I think the name isModalMessageBoxBlocking might even be better.

It raises the question, what is it blocking?

@seanbudd seanbudd changed the title Implement API changes for gui.message Replace boolean gui.IsInMessageBox with function gui.message.isModalMessageBoxActive Mar 2, 2022
@seanbudd seanbudd requested a review from feerrenrut March 2, 2022 03:06
source/gui/message.py Outdated Show resolved Hide resolved
source/gui/message.py Outdated Show resolved Hide resolved
source/gui/message.py Outdated Show resolved Hide resolved
source/gui/message.py Outdated Show resolved Hide resolved
source/gui/message.py Outdated Show resolved Hide resolved
@seanbudd seanbudd requested a review from feerrenrut March 2, 2022 23:23
@seanbudd seanbudd merged commit 9fbcbc0 into master Mar 3, 2022
@seanbudd seanbudd deleted the guiMessageAPI branch March 3, 2022 22:59
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