-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Saving/restoring checked state is off by default Restoring checked state caused many issues for groupboxes used in same window, but for different parent objects (e.g. labeling for different layers). - Saving/restoring checked state can be get/set via code - Add ability to get/set a QSettings group (instead of window obj name) for saving states. Allows for groupboxes to be used in multiple places/dialogs and maintain same states (e.g. adv labeling dialog).
- Loading branch information
Showing
3 changed files
with
31 additions
and
14 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
58ba3f0
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 Larry - a few comments
I still fail to understand the need for this (disregarding the fact that's it's buggy).
Where are "multiple instances of the same groupbox" and why not separate the instances?
To address the save key issue, you could use window+parent+groupbox instead of manually adding a group name.
58ba3f0
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 Etienne,
Let me explain further so you understand why these changes are important.
Checkbox save/restore off by default
It is imperative that the checkbox not be restored unless the developer has specifically turned it on for the groupbox, or automatic adjustment (and/or loss) of settings can occur.
Example: Adv Labeling dialog
User has set checkable groupbox options in Adv Labeling dialog for VectorLayer A, e.g. Buffer or Scale Visibility. User closes that vector layer options dialog and opens another for VectorLayer B. The newly opened dialog has Buffer, etc. automatically checked, even though the user did not decide that yet. Much worse, the user decides they don't need those and unchecks Buffer, etc. Now when the user re-opens VectorLayer A's options dialog Buffer, etc. are now unchecked (VERY, VERY BAD). If the user doesn't notice, and clicks Apply or Save, they will effective clear (lose) those auto-unchecked settings.
Restoring the checked aspect of the groupbox can NOT be the default, unless the developer of the dialog knows the risks and feels it is not an issue...
Example: Raster Save As...
in the constructor for the dialog will restore previous functionality. Restoring the checked state of the groupboxes in that dialog is 'safe' because it is for specifically for output, not for setting options for different objects (as with VectorLayer A and B, above).
Custom group name for QSettings
This optional setting allows a groupbox or groupboxes in a base widget to be used in multiple places (e.g. Adv Labeling in its standalone dialog or embedded in Vector Layer Options) while maintaining the exact same collapsed/expanded (and optionally checked state), regardless of its parent or parent window. This is done for the Adv Labeling base dialog by simply adding the following to the dialog's init() method...
If a developer feels the optional custom name is not needed, the default behavior is to use the window object name as before. There is no way to automate a QSettings group string since any base groupbox could be embedded as a standalone widget, thereby having a completely different parent tree. This is a convenience method to solve that problem.
I have fully tested these settings and there are no apparent bugs. The checked state of checkable groupboxes in Adv Labeling will maintain their state relative to a given layer's PAL labeling options (which has nothing to do with QgsCollapsibleGroupBox). Only the collapsed/expanded state of groupboxes will be saved/restored by the QgsCollapsibleGroupBox class. This is expected and correct behavior, with no effect upon the options.
You will need to add the grpbox->setSaveCheckedState( true ) foreach loop to the constructor of QgsRasterLayerSaveAsDialog, at least after
setupUi( this );
, to bring back previous saving/restoring of checked state.Saving/restoring the collapsed/expanded state by default is fine, and I don't see a reason to offer to individually turn it off. Having setSaveState() allows for turning off all saving/restoring, or setSaveCheckedState() to optionally turn on for checked state, both of which make sense. I don't see a need to rename or change anything further.
Regards,
Larry
58ba3f0
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 Larry
I understand now, thanks for the detailed explanation.
I guess what I thought was a bug is because in the advLabelling the checked state of some groupboxes depends on the layer, and gets reset when closing qgis.
However, I still think it makes more sense to have a separate setting for save/restore the collapse state and check state. I don't find it intuitive that 2 settings would impact one property (save/restore checkbox), whereas only one for collapse.
In summary, I would prefer by default mSaveCollapsedState=true and mSaveCheckedState=false, both of which can be overridden and there is no confusion at all.
In the case of the save as dialog, most boxes won't be restored (such as Create Options), because a checked options is useless until user adds information.
I'll commit these small changes, unless you object about them.
58ba3f0
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.
actually I'll add some code to the create options to remember previous options also, and remember checked state. cheers
58ba3f0
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.
Ok. Sounds fine to me. You're right, it will probably be better to have the properties and get/set methods as clear as possible. I'll hold off tinkering with the class anymore, until I can test your next commit. Thanks!