-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Changed code that causes inconsistency in layouts #20141
base: 5.x
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @dan123456-eng, thanks a lot for your contribution! I'm afraid this still needs more work (see below).
spyder/plugins/layout/api.py
Outdated
plugin.dockwidget.show() | ||
plugin.toggle_view_action.setChecked(True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By commenting these two lines you're preventing external plugins to be shown when switching layouts, which is basically what this for
loop is doing.
So, you need to find a better solution that preserves this functionality and fixes the bug you found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ccordoba12! I'm here working on a better solution that preserves things correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ccordoba12 how are you? Is the purpose of this loop to keep the third panel (if checked to stay open), eg when I switch from Rstudio layout to Matlab layout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, that's the idea! The problem before I added this loop was that no third-party plugin was shown when switching between different layouts.
But the bug it introduced is that it's not respecting plugins that are declared as not visible in the layout declaration. That must preserved when switching layouts as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, that's exactly what I thought. I'll keep looking for a solution here and let you know. Tks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the bug. I'll update the PR soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dan123456-eng, I'm afraid this still needs more work (see below).
external_plugins_to_show.append(plugin.NAME) | ||
if (plugin.NAME in PLUGIN_REGISTRY.external_plugins): | ||
for plugin in dockable_plugins: | ||
external_plugins_to_show.append(plugin.dockwidget.isVisible()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list (i.e. external_plugins_to_show
) needs to contain the plugin names that are going to be restored in the for
loop below. However, now you're saving on it boolean values (i.e. True
or False
) according to the plugin's visibility.
This change doesn't generate an error because we're using
plugin = main_window.get_plugin(plugin_id, error=False)
in line 503, which simply returns None
if there are any errors when trying to return the plugin instance from its name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I uploaded it this way because here using the "visible = False or True" part works correctly and keeps the plugins when switching Spyder layouts.
The way it was previously restored the panels by "plugin.NAME" no matter if it was "visible = False or True".
Now I will try to use "plugin.NAME" and check if it is "visible = False or True".
Sorry for the delay to answer you. I had to do some university stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I will try to use "plugin.NAME" and check if it is "visible = False or True".
Ok, I think that idea could work.
Sorry for the delay to answer you. I had to do some university stuff.
No problem, I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ccordoba12 I did it as follows and tested it, everything worked correctly. When its can take a look and if everything is ok I'll go up to PR. Tks!
`# Stores boolean values of panels
external_plugins_value = []
# External dockable plugins to show after the layout is applied
external_plugins_to_show = []
# Before applying a new layout all plugins need to be hidden
for plugin in dockable_plugins:
all_plugin_ids.append(plugin.NAME)
# Save currently displayed external plugins
if (plugin.NAME in PLUGIN_REGISTRY.external_plugins and plugin.dockwidget.isVisible()):
for plugin in dockable_plugins:
external_plugins_value.append(plugin.dockwidget.isVisible())
plugin.toggle_view(False)
# Checks if the value is true
for value in external_plugins_value:
if value == True == plugin.dockwidget.isVisible():
external_plugins_to_show.append(plugin.NAME)
plugin.toggle_view(False)`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some issues with the solution you proposed above:
-
This
if
statementif (plugin.NAME in PLUGIN_REGISTRY.external_plugins and plugin.dockwidget.isVisible()): for plugin in dockable_plugins: external_plugins_value.append(plugin.dockwidget.isVisible())
is adding an entry per dockable plugin to the
external_plugins_value
list with its visibility state. My question is: why is it necessary to iterate over all dockable plugins here? -
This
for
loop# Checks if the value is true for value in external_plugins_value: if value == True == plugin.dockwidget.isVisible(): external_plugins_to_show.append(plugin.NAME)
is not adding any plugin to
external_plugins_to_show
because before it you have the commandplugin.toggle_view(False)
, which changes the visibility ofplugin
. In other wordsplugin.dockwidget.isVisible()
will always returnFalse
in the lineif value == True == plugin.dockwidget.isVisible():
So I don't think it'd work either, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few problems with the solution you proposed above:
This statement
if
if (plugin.NAME in PLUGIN_REGISTRY.external_plugins and plugin.dockwidget.isVisible()): for plugin in dockable_plugins: external_plugins_value.append(plugin.dockwidget.isVisible())is adding a plug-in entry that is in place to the list with its visibility state. My question is: why is it necessary to iterate over all the plugins that fit here?
external_plugins_value
here i do the iteration to get all true and false values from external plugins and store in external_plugins_value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop
for
# Checks if the value is true for value in external_plugins_value: if value == True == plugin.dockwidget.isVisible(): external_plugins_to_show.append(plugin.NAME)is not adding any plugin for because before it you have the command , which changes the visibility of the . In other words, it will always return on the line
external_plugins_to_show``plugin.toggle_view(False)``plugin``plugin.dockwidget.isVisible()``False
if value == True == plugin.dockwidget.isVisible():So I don't think it would work either, sorry.
here I add plugin.NAME only if True ("if open") external_plugins_value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't think it would work either, sorry.
Thank you for your help and for your patience.
@dan123456-eng, could you better explain what you're trying to achieve in this pull request? I don't understand it very well but I think if I did, I think I could give you better guidance. |
In this PR I'm trying to add in "external_plugins_to_show.append(plugin.NAME)" only plugin.NAME that visible==True.
In the code above "plugin.NAME" is added in "external_plugins_to_show.append(plugin.NAME)" both visible=True and visible=False. |
Description of Changes
Change in the code that causes the problem
#20078.
Modified code:
plugin.blockSignals(True)
#plugin.dockwidget.show()
#plugin.toggle_view_action.setChecked(True)
plugin.blockSignals(False)`
Issue(s) Resolved
Fixes #20078