Skip to content

(issue 165) remember size of main window and internal components #191

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

Merged
merged 2 commits into from
Oct 10, 2019

Conversation

alixdamman
Copy link
Contributor

No description provided.

@alixdamman alixdamman changed the title 165 remember size (issue 165) remember size of main window and internal components Sep 6, 2019
Copy link
Collaborator

@gdementen gdementen left a comment

Choose a reason for hiding this comment

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

Forget about get_widgets_with_state_to_restore. To implement it, you would need too keep track of the widgets in instance variables within the __init__ method, so directly storing them in the dict like you do now is better.

@@ -263,13 +263,20 @@ def _setup_and_check(self, widget, data, title, readonly, **kwargs):
main_splitter.addWidget(comparatorwidget)
main_splitter.setSizes([5, 95])
main_splitter.setCollapsible(1, False)
self.widget_state_settings['{}_main_splitter'.format(self.name)] = main_splitter
Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't it be cleaner if you didn't use self.name in the dict keys (but added it in save_widgets_state_and_geometry and restore_widgets_state_and_geometry)?

@gdementen
Copy link
Collaborator

One more thing: remember most of (all in this case) my review comments are suggestions of how I think we could do better. You are free to disagree that it is actually better (and don't need to explain why).

@alixdamman
Copy link
Contributor Author

In one of your previous comment:

Could you explain me why you need this two-step process (ie add widgets to a dict and then traverse that dict) instead of doing that in a "get_widgets_with_state_to_restore" method?

I am not sure of how to implement the "get_widgets_with_state_to_restore()" method?
Is it a method that needs to be override in subclasses manually or a more general method that will dig into the main and sub layouts to detect all widgets with a saveState() method?

@gdementen
Copy link
Collaborator

I am not sure of how to implement the "get_widgets_with_state_to_restore()" method?
Is it a method that needs to be override in subclasses manually or a more general method that will dig into the main and sub layouts to detect all widgets with a saveState() method?

As I said above, forget about it. I meant the manual subclass override but what you have now is simpler anyway.

@alixdamman alixdamman added this to the 0.32 milestone Oct 10, 2019
@alixdamman
Copy link
Contributor Author

OK to merge?

Copy link
Collaborator

@gdementen gdementen left a comment

Choose a reason for hiding this comment

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

Ohhh, shit! Another one of my review comments I wrote several weeks ago and forgot to submit... Sorry.

def save_widgets_state_and_geometry(self):
settings = QSettings()
settings.beginGroup(self.settings_group_name)
settings.setValue('geometry/{}'.format(self.name), self.saveGeometry())
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't using self.name redundant now that you are using beginGroup(self.settings_group_name)? (and it causes the resulting string to mix technical/lowercase strings with "user friendly" strings).

@gdementen
Copy link
Collaborator

LGTM (modulo history cleanup of course), nice work!

@alixdamman alixdamman merged commit 9d4f835 into larray-project:master Oct 10, 2019
@alixdamman alixdamman deleted the 165_remember_size branch October 10, 2019 14:03
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