-
Notifications
You must be signed in to change notification settings - Fork 203
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
Add menu in Hierarchy View to reset the model and variant selections #634
Conversation
</item> | ||
</layout> | ||
</item> | ||
<item row="4" column="0"> |
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.
.ui file will be hard to review, but the reason it looks like so much has changed in the diff is because the new menu item goes at the top, so it becomes row 0. This pushes all of the other items down a row and ends up creating a diff like this.
I found it harder than expected to add the menu bar + item at the top of the dialog, there is no interactive way of doing this in Qt Designer as far as I can tell unless we change the base class type of the dialog to a window. I did find a way to add the menu bar as a widget to a layout and ended up basically hand-editing this file to make it work.
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.
Okay as long as it still works. Sometimes even when editing in Qt Designer and adding one thing the diff was all messed up.
@@ -71,6 +71,7 @@ USDImportDialog::USDImportDialog(const std::string& filename, const ImportData* | |||
fUI->treeView->setAlternatingRowColors(true); | |||
fUI->treeView->setSelectionMode(QAbstractItemView::SingleSelection); | |||
QObject::connect(fUI->treeView, SIGNAL(clicked(const QModelIndex&)), this, SLOT(onItemClicked(const QModelIndex&))); | |||
QObject::connect(fUI->actionReset_File, SIGNAL(triggered(bool)), this, SLOT(onResetFileTriggered())); |
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.
Hooking up the menu item action to the reset function. It's not a check-box menu so I'm ignoring the bool state variable.
lib/usd/ui/TreeModel.cpp
Outdated
assert(variantItem); | ||
|
||
if (variantItem->variantSelectionModified()) | ||
{ | ||
resetVariantToPrimSelection(variantItem); | ||
} |
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.
Here you assert, but then just use variantItem. So if it happened to be nullptr you would crash and thus never get to the call to resetVariantToPrimSelection where inside you have an if(variantItem) check. I would be more inclined to put the nullptr check on line 86 and move the assert into resetVariantToPrimSelection and remove the if check there. This makes resetVariantToPrimSelection a helper that is assuming valid input.
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.
Yeah, good point. Will change.
lib/usd/ui/USDImportDialog.cpp
Outdated
@@ -172,4 +173,13 @@ void USDImportDialog::onItemClicked(const QModelIndex& index) | |||
} | |||
} | |||
|
|||
void USDImportDialog::onResetFileTriggered() | |||
{ | |||
if (fTreeModel) |
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.
What is our coding standard stance on if checks? In Maya only a bool is a boolean, so if the type is not a boolean don't use it as one.
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.
Not sure if we've listed this in the coding standard or not, but I can add a "nullptr !=" here.
</item> | ||
</layout> | ||
</item> | ||
<item row="4" column="0"> |
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.
Okay as long as it still works. Sometimes even when editing in Qt Designer and adding one thing the diff was all messed up.
New menu item in the Hierarchy View dialog that will reset all of the edited variant sets back to what is in the usd file, and sets the checked state back to the root selection