Replace boolean gui.IsInMessageBox with function gui.message.isModalMessageBoxActive#13376
Conversation
feerrenrut
left a comment
There was a problem hiding this comment.
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")|
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? |
feerrenrut
left a comment
There was a problem hiding this comment.
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?
I don't believe this is true, as per #12984 (comment). I think the name |
|
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.
It raises the question, what is it blocking? |
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.messagehad 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.IsInMessageBoxwas a boolean, this has been replaced by a functiongui.message.isModalMessageBoxActive, which tracks if a messageBox is open in a safer manner.Changes logic
gui/message.pyto 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: