-
Notifications
You must be signed in to change notification settings - Fork 100
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
Skyline: Improved Import Results Files form restoring UI from last use. #3199
Skyline: Improved Import Results Files form restoring UI from last use. #3199
Conversation
…e. (requested by Brandon)
Looks like you forgot to add "ImportResultsFilesRestoreTest.cs" |
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.
Looks good to me. Definitely makes the experience better.
I wonder if we should set "MinimumSize" on the OpenDataSourceDialog just to prevent the user from making it ridiculously small. Now that the size gets remembered it's conceivable a user could get themselves in a situation where the OpenDataSourceDialog is too small for them to know what to do.
I wonder if all the SrmResultsXxx properties should be combined together into a single class with several properties instead of being separate properties on the Settings object.
listView.Sort(); | ||
private void ToggleListViewSort(int columnIndex) | ||
{ | ||
var order = SortOrder.Ascending; |
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.
In the regular File Open dialog, the default initial sort order depends on the column.
It's SortOrder.Descending for the Date column.
I wonder whether we should be duplicating that logic here.
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 for this PR
@@ -886,6 +886,21 @@ | |||
</Setting> | |||
<Setting Name="RelativeAbundanceLogScale" Type="System.Boolean" Scope="User"> | |||
<Value Profile="(Default)">True</Value> | |||
</Setting> | |||
</Setting> | |||
<Setting Name="SrmResultsSavedForPath" Type="System.String" Scope="User"> |
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.
I think it would be safe to rename "SrmResultsXxx" properties if we think a different naming scheme would be better ("ImportResultsXxx", or "OpenDataSourceXxx"?)
"SrmResultsDirectory" was never actually used in older versions of Skyline because it only applied if the document had never been saved, so we don't have to worry about backwards compatibility.
"SrmResultsSourceType" was the only SrmResultsXxx property that actually got used.
- fix loss of ability to set the starting path in ImportResultsDIAControl.Browse()
….com:ProteoWizard/pwiz into Skyline/work/20241016_import_results_restore
(requested by Brandon)