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

Fix registered targets lost when re-launching on existed server #355

Merged
merged 3 commits into from
Feb 11, 2020

Conversation

davidlatwe
Copy link
Collaborator

This was reported in Gitter 👉 February 3, 2020 4:54 PM from @BigRoy .

It was a bug after #347, it was trying to solve #335 which was assigning and changing targets via pyblish_qml.api.show. But the solution did not pay attention to the case that is using targets from pyblish.api.registered_targets, hence we got the bug.

Reproducible

import pyblish.api
import pyblish_qml.api

# Register some random target.
pyblish.api.register_target("custom")
# Launch !
pyblish_qml.api.show()
# Now navigate to the `Terminal` page on top of GUI, should see our target "custom" in view.
# While window still exists, launch again.
pyblish_qml.api.show()
# Targets has been wiped..

What's changed

  • Only updating targets to current server if targets is provided
  • Default value of targets changes to None instead of mutable list

@BigRoy
Copy link
Member

BigRoy commented Feb 3, 2020

Nice - thanks for looking into it so quickly.

This could be a tricky one though. What happens on:

pyblish_qml.show(targets=["custom"])
# ... keep window open
pyblish_qml.show(targets=None)

Would we want the latter to resort to the default of using [default] + api.registered_targets() like on the initial run or to have it persist whatever it had.

It seems most logical if it behaved the same to a complete refresh (close and run) with the same arguments. As such, should it on targets=None be targets = [default] + api.registered_targets() as is done on __init__. I'd expect that to be the most logical behavior. Does it currently behave like that?

@davidlatwe
Copy link
Collaborator Author

Does it currently behave like that?

No ! And I think you are right, that would be what I'll expected, too.

@davidlatwe
Copy link
Collaborator Author

Does it currently behave like that?

And now it does.

Previously, targets provided from pyblish_qml.api.show will remain the same if next show call has no targets given. After 3fdccb5, it will fallback to [default] + pyblish.api.registered_targets().

Copy link
Member

@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.

This fixes the issue - thanks a lot! 🚀

@davidlatwe
Copy link
Collaborator Author

Thanks ! Will merge this later today :D

@davidlatwe davidlatwe merged commit 692965a into pyblish:master Feb 11, 2020
@davidlatwe davidlatwe deleted the fix-targets branch February 11, 2020 04:31
@davidlatwe
Copy link
Collaborator Author

Arrr sorry, forgot to bump the version, will push to master. 😭

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