-
Notifications
You must be signed in to change notification settings - Fork 110
Properties View #3573
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
base: Skyline/work/20250102_FilesView
Are you sure you want to change the base?
Properties View #3573
Conversation
…properties and descriptors for use in the property sheet.
This reverts commit 77133b9.
This reverts commit 0ad43ea.
…0530_PropertiesView
…es, whose list items get nested in the property grid.
…teoWizard/pwiz into Skyline/work/20250530_PropertiesView
…name-accessed resources.
… property grid, code will throw an error if there is not an associated display name. This is necessary to write tests to ensure the property grid is properly displaying, even when someone changes a variable name.
…nd one file for each control object. Must pass the right resource manager into AddPropertiesFromAnnotatedObject. AddPropertiesFromAnnotatedObject broken up with private helper functions for clarity.
…rtyObject. Added code inspection to ensure that all properties have a matching property name resource, to avoid issues when a property name is changed.
…0530_PropertiesView
…teoWizard/pwiz into Skyline/work/20250530_PropertiesView
…nd have each one handle its selectionchanged event to get the exact selected object. Also changed replicateProperties category flags to "replicate" from "chromatagram".
…iblity is saved between skyline runs
…r IPropertySheetOwner
…tchangedui instead of documentchanged.
Use IDocumentContainerUI.ListenUI instead of Listen on PropertyGridForm to listen to document change events. Remove "lock(SkylineWindow.GetDocumentChangeLock()) { }" calls.
…rtyGridTest are incomplete.
….com:ProteoWizard/pwiz into Skyline/work/20251009_PropertiesView_Testing
f9698f3
to
c53e358
Compare
…nctionality between it and FilesTree Properties to separate Test Util file.
Add class "BuiltInReports" which contains the logic for deciding which columns should be displayed in the default reports in the Document Grid. This logic used to be in SkylineViewContext.GetDefaultViewInfo. Moving it into a new class will make it easier to be called from Aaron's Property Sheet code.
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.
Good to see PropertiesView coming together. Nice work. Left several comments - some nits and others about API design. Let me know if you have questions or want to discuss.
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 don't see these in the FilesView PR. Should these be here and could this cause a problem merging back to main?
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.
This was something I wanted to ask about, I might need help resolving this. They appeared in the git changes after I force reset from another test branch, and for some reason there was no option to undo the changes. It seems like some kind of buggy artifact, but I also tried cleaning and rebuilding to no luck. Maybe Nick has some ideas on how to resolve it, I can ask him.
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 ran into this too and iirc, resolved it using 'git submodule update'. Try updating submodules locally, though you may need to remove these files first? I think this was related to Matt's checkin, so he may have advice on how to fix.
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 I did that before, git submodule update resolved the local changes but didn't change anything on github when I pushed. I will email Matt
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 don't think this has anything to do with any of my commits. Have you tried reverting the submodule locally and then committing it to effectively revert this change on the PR?
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.
Thanks, I was able to resolve it
[TestMethod] | ||
public void TestSequenceTreePropertyGrid() | ||
{ | ||
TestFilesZip = SequenceTreeRatioTest.TEST_FILES_ZIP; |
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.
Is Nick ok with sharing SequenceTreeRatioTest's zip file?
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, I just went with the same strategy I did for FilesTreePropertyGridTest of using a zip that was already in use. I can ask him.
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.
Thought that might be the case. :) I remember us doing that for Files. That said, it's a pattern I wouldn't repeat since it creates a dependency between two unrelated things - making each harder to evolve. I considered changing FilesTreeFormTest.zip recently and held off to avoid breaking this. Might still change its this week, though. ;)
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.
So would the correct strategy be to copy the zip and rename it to something specific to my test?
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.
Yes, that's a good approach and avoids tight coupling between unrelated tests. Avoid reusing the FilesTreeFormTest.zip (which is 90MB and much larger than is typical). Instead (1) use something smaller and (2) use a .data directory rather than a .zip file. The former is easier to maintain. FilesTreeFileSystemTest is an example of both #1 and #2.
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's up with this code? There's no such file, but there is one with a .data suffix. Is TestFunctional doing something weird behind the scenes?
TestFilesZipPaths = new[] { @"TestFunctional\FilesTreeFileSystemTest.zip" };
pwiz_tools/Skyline/TestFunctional/SequenceTreePropertyGridTest.cs
Outdated
Show resolved
Hide resolved
…tyGridTest and SequenceTreePropertyGridTest share the same test folder. Also fixed reliance on SelectNode functions, moving to setting SelectedNode = .... directly in tests.
Adding a properties view analog to Visual Studio's properties view. Clicking on a relevant control, like a FileNode in Files View, will display its properties. Properties view is a dockable form.