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

Make debug and show behaviour consistent for all tools #513

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jasperges
Copy link
Contributor

@jasperges jasperges commented Jan 16, 2020

What's changed?
This implements #512. In this PR I have included the override for sys.excepthook, but that can be removed easily.

@jasperges jasperges changed the title Make debug behaviour consistent Make debug behaviour consistent for all tools Jan 16, 2020
@jasperges
Copy link
Contributor Author

For people reading this: after this PR there is no tool which calls special code when in debug mode. For now I have a simple if block:

if debug:
    # Add code here that only runs when debugging is enabled.
    pass

Should I keep this or remove it completely? The only reason to keep it would be so that it's super clear that code can be run only for debug mode. In case someone wants to add debug code for the tools.

@tokejepsen
Copy link
Collaborator

Hmm, if none of the tools have any debug code, maybe just remove it all?

@jasperges
Copy link
Contributor Author

As mentioned here the Loader tool handles an existing module window in a different way. Although it's not directly related to the debug behaviour, I could change that code too, so it's consistent with the other tools.

@BigRoy it seems you implemented this. Did you have a specific reason for it? Do you think it's fine to change it?

@BigRoy
Copy link
Collaborator

BigRoy commented Feb 6, 2020

@jasperges I can't recall. Looking at the code it seems to solve the issue where the Window might have been garbage collected. I can't recall an actual case of when/where that might have happened. Looking at the blame it seems the original try..except statement came from three years ago by @mottosso

I guess I just ended up preserving that behavior. Thus no need to keep it around for me if it works without it.

@jasperges
Copy link
Contributor Author

@BigRoy Okay, then I will remove it and make it consistent with the other tools.

@jasperges jasperges changed the title Make debug behaviour consistent for all tools Make debug and show behaviour consistent for all tools Feb 17, 2020
@jasperges
Copy link
Contributor Author

jasperges commented Feb 17, 2020

This whole thing got a little bit bigger then initially intended. Do you think this is okay or would it be better to got back to just making the debug behaviour consistent and then open another PR for getting all show related code consistent?

To be honest I have not tested this properly yet. I will do that asap.

@BigRoy
Copy link
Collaborator

BigRoy commented Feb 17, 2020

To me the current changes don't seem too large for a single PR. We'll just need to make sure things work as we intended and we're good to go.

Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

I think the simplified code is good for the code base.

I do feel that it's a minor loss that clicking "Avalon > Load.." again in for example Maya that after this change it will close any existing loader window if it's open and show a new one (meaning it's reset to default state!). Where previously it would just show and raise the existing window.

I guess if everyone kind of agrees that it's the preferred change then let's get this merged.

Copy link
Collaborator

@davidlatwe davidlatwe left a comment

Choose a reason for hiding this comment

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

Looks like try closing module.window on show was adopting from sceneinventory and projectmanager.

For loader app, it's a minor loss indeed. But in return is simplicity for all DCC App/OS integrations, so +1 for this change :P

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.

4 participants