Skip to content

Commit

Permalink
Update to QgsCollapsibleGroupBox
Browse files Browse the repository at this point in the history 
- 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
@dakcarto
dakcarto committed Sep 22, 2012
1 parent bf985a5 commit 58ba3f0
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 14 deletions.
15 changes: 6 additions & 9 deletions src/app/qgslabelinggui.cpp
View file
Expand Up @@ -268,15 +268,12 @@ QgsLabelingGui::QgsLabelingGui( QgsPalLabeling* lbl, QgsVectorLayer* layer, QgsM
connect( quadrantRadios[i], SIGNAL( toggled( bool ) ), this, SLOT( updateQuadrant() ) );
}

// Label tab collapsed groupboxes
chkBuffer->setCollapsed( true );
mFontMultiLineGroupBox->setCollapsed( true );
chkFormattedNumbers->setCollapsed( true );
chkScaleBasedVisibility->setCollapsed( true );

// Data defined tab collapsed groupboxes
mBufferAttributesPropertiesGroupBox->setCollapsed( true );
mFontAttributePropertiesGroupBox->setCollapsed( true );
// Global settings group for groupboxes' saved/retored collapsed state
// maintains state across different dialogs
foreach ( QgsCollapsibleGroupBox *grpbox, findChildren<QgsCollapsibleGroupBox*>() )
{
grpbox->setSettingGroup( QString( "mAdvLabelingDlg" ) );
}

connect( groupBox_mPreview,
SIGNAL( collapsedStateChanged( QgsCollapsibleGroupBox* ) ),
Expand Down
20 changes: 15 additions & 5 deletions src/gui/qgscollapsiblegroupbox.cpp
View file
Expand Up @@ -52,6 +52,10 @@ void QgsCollapsibleGroupBox::init()
// variables
mCollapsed = false;
mSaveState = true;
// NOTE: only turn on mSaveCheckedState for groupboxes NOT used
// in multiple places or used as options for different parent objects
mSaveCheckedState = false;
mSettingGroup = ""; // if not set, use window object name
mInitFlat = false;
mScrollOnExpand = true;
mShown = false;
Expand Down Expand Up @@ -167,7 +171,8 @@ QString QgsCollapsibleGroupBox::saveKey() const
// }
// if ( parent() != NULL )
// saveKey = "/" + parent()->objectName() + saveKey;
saveKey = "/" + window()->objectName() + saveKey;
QString setgrp = mSettingGroup.isEmpty() ? window()->objectName() : mSettingGroup;
saveKey = "/" + setgrp + saveKey;
saveKey = "QgsCollapsibleGroupBox" + saveKey;
return saveKey;
}
Expand All @@ -181,9 +186,13 @@ void QgsCollapsibleGroupBox::loadState()

QSettings settings;
QString key = saveKey();
QVariant val = settings.value( key + "/checked" );
if ( ! val.isNull() )
setChecked( val.toBool() );
QVariant val;
if ( mSaveCheckedState )
{
val = settings.value( key + "/checked" );
if ( ! val.isNull() )
setChecked( val.toBool() );
}
val = settings.value( key + "/collapsed" );
if ( ! val.isNull() )
setCollapsed( val.toBool() );
Expand All @@ -197,7 +206,8 @@ void QgsCollapsibleGroupBox::saveState()
return;
QSettings settings;
QString key = saveKey();
settings.setValue( key + "/checked", isChecked() );
if ( mSaveCheckedState )
settings.setValue( key + "/checked", isChecked() );
settings.setValue( key + "/collapsed", isCollapsed() );
}

Expand Down
10 changes: 10 additions & 0 deletions src/gui/qgscollapsiblegroupbox.h
View file
Expand Up @@ -44,6 +44,14 @@ class GUI_EXPORT QgsCollapsibleGroupBox : public QGroupBox

//! set this to false to not save/restore check and collapse state
void setSaveState( bool save ) { mSaveState = save; }
//! set this to true to save/restore checked state
/** @note only turn on mSaveCheckedState for groupboxes NOT used
* in multiple places or used as options for different parent objects */
void setSaveCheckedState( bool save ) { mSaveCheckedState = save; }
bool saveCheckedState() { return mSaveCheckedState; }
//! set this to a defined string to share save/restore collapsed state across dialogs
void setSettingGroup( const QString &group ) { mSettingGroup = group; }
QString settingGroup() const { return mSettingGroup; }
//! set this to false to not automatically scroll parent QScrollArea to this widget's contents when expanded
void setScrollOnExpand( bool scroll ) { mScrollOnExpand = scroll; }

Expand Down Expand Up @@ -71,6 +79,8 @@ class GUI_EXPORT QgsCollapsibleGroupBox : public QGroupBox

bool mCollapsed;
bool mSaveState;
bool mSaveCheckedState;
QString mSettingGroup;
bool mInitFlat;
bool mScrollOnExpand;
bool mShown;
Expand Down

5 comments on commit 58ba3f0

@etiennesky
Copy link
Contributor

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

  • why is saving check state off by default and not on? Now none of the checkboxes get restored to their checked on state (in raster save as as well as in advLabelling) after closing dialog and re-opening the dialog. Although if I close qgis and re-open then the advLabelling restores check state (but not save as)
  • you should rename setSaveState to setCollapseCollapseState for consistency - or am I missing something?

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.

@dakcarto
Copy link
Member Author

Choose a reason for hiding this comment

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

Hi Etienne,

  • why is saving check state off by default and not on? Now none of the checkboxes get restored to their checked on state (in raster save as as well as in advLabelling) after closing dialog and re-opening the dialog. Although if I close qgis and re-open then the advLabelling restores check state (but not save as)
  • you should rename setSaveState to setCollapseCollapseState for consistency - or am I missing something?

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.

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

  foreach ( QgsCollapsibleGroupBox *grpbox, findChildren<QgsCollapsibleGroupBox*>() )
    grpbox->setSaveCheckedState( true );

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

  foreach ( QgsCollapsibleGroupBox *grpbox, findChildren<QgsCollapsibleGroupBox*>() )
    grpbox->setSettingGroup( QString( "mAdvLabelingDlg" ) );

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

@etiennesky
Copy link
Contributor

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.

@etiennesky
Copy link
Contributor

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

@dakcarto
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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!

Please sign in to comment.