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

Implements Tri-State checkboxes on Tailoring screen. #113

Merged
merged 2 commits into from
May 11, 2017
Merged

Implements Tri-State checkboxes on Tailoring screen. #113

merged 2 commits into from
May 11, 2017

Conversation

WesleyCeraso
Copy link
Contributor

Closes #112

Signed-off-by: Wesley Ceraso Prudencio wcerasop@redhat.com

Closes #112

Signed-off-by: Wesley Ceraso Prudencio <wcerasop@redhat.com>
@WesleyCeraso
Copy link
Contributor Author

Also fixes #36, #23

@WesleyCeraso
Copy link
Contributor Author

@openscap-jenkins retest this please

@rsprudencio
Copy link
Contributor

@openscap-jenkins retest this please

@mpreisler mpreisler added this to the 1.1.5 milestone May 10, 2017
@mpreisler
Copy link
Member

This is pretty cool and seems to be working well. It is however very slow. Could we make it faster? I suspect it's probably the repeated wasted redraws that's making it slow.

If we can't make it faster we should at least show a busy cursor to let the user know we are doing something and the app hasn't crashed.

@mpreisler
Copy link
Member

@yuumasato I would appreciate your review here as well. This is a big change.

@WesleyCeraso
Copy link
Contributor Author

@mpreisler I haven't noticed how slow it was when painting big trees. In fact it was really slow when a big part of the tree is expanded and you check/uncheck the most top item. It's now fixed.

@@ -268,6 +268,7 @@ void TailoringWindow::synchronizeProfileItem()
void TailoringWindow::synchronizeTreeItem()
{
synchronizeTreeItemSelections(mBenchmarkItem);
return;
Copy link
Member

Choose a reason for hiding this comment

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

?

@@ -315,6 +322,7 @@ void TailoringWindow::synchronizeTreeItemSelections(QTreeWidgetItem* treeItem)
treeItem->setCheckState(0, Qt::Unchecked);
}
break;

Copy link
Member

Choose a reason for hiding this comment

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

?

@mpreisler
Copy link
Member

It's faster now but when I select/deselect it screws up the scroll position and scrolls all the way up.

@mpreisler
Copy link
Member

mpreisler commented May 10, 2017

It's much better to just disable repainting while you change the tree. That's easier than collapsing and expanding parts of the tree. I think it's setUpdatesEnabled or something like that.

@WesleyCeraso
Copy link
Contributor Author

@mpreisler Yeah, I'll take take a better look at this.

@WesleyCeraso
Copy link
Contributor Author

@mpreisler The problem was not exactly with the painting event, as it can be postponed by setUpdatesEnabled(bool enable), instead, if the treewidget was visible, for each data changed it would recalculate its geometry.
The only way I found to prevent it is turning the treewidget invisible. The last solution I gave (collapsing) did the same thing but would lost the scroll position as you noticed.

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

These checkboxes are really cool.
I noticed that the default profile shows empty groups as selected, I think they should be not selected.

// https://github.com/qt/qt/blob/4.8/src/gui/itemviews/qabstractitemview.cpp#L3190
mUI.itemsTree->setVisible(false);
synchronizeTreeItemSelections(mBenchmarkItem);
mUI.itemsTree->setVisible(true);
Copy link
Member

Choose a reason for hiding this comment

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

Please, make it clearer that we are making it invisible and visible again due to perceived slowness when changing state of long trees.

Improves speed when the tree is expanded and a top item is checked.

Signed-off-by: Wesley Ceraso Prudencio <wcerasop@redhat.com>
@WesleyCeraso
Copy link
Contributor Author

WesleyCeraso commented May 11, 2017

@yuumasato Empty groups are checked by default so that their parents can be checked, instead of partially checked, in the case all their siblings are checked as well.

@yuumasato
Copy link
Member

@WesleyCeraso That makes sense.
These are great changes, thank you!
Ack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants