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

DependencyGraphViewer: Feature add same window navigation for NodeForm #84542

Merged

Conversation

mitchcapper
Copy link
Contributor

Add ability to reuse current window rather than open new, added history (forward/back) and arrow key accelerators as well.

Makes it far easier to go through a massive stack of hundred+ without opening 100 windows.

If this change is desired I can add some help text (i assume on the primary button? or???) .

Been a long time since I have touched winforms so if something isn't done the right away let me know.

Also, due to some winforms issues (ie #84539) and its design to reformat everything on save I hand coded most of the changes on in to the designer side.

Add ability to reuse current window rather than open new, added history
(forward/back) and arrow key accelerators as well.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 9, 2023
@mangod9 mangod9 requested a review from davidwrighton May 1, 2023 17:03
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

I like the change, but I would actually like to see the reformatted file to match the current designer code-spit. It will make future changes less problematic. Alternatively, if you want to push forth a PR just to do the refactoring to what the designer does, and then layer on your changes, I would accept that pair of PR's as well.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 1, 2023
@davidwrighton
Copy link
Member

And, this codebase is not a good example of Winforms structure. I wrote it up in a fit of grumpiness to solve a single problem years ago, and it has just evolved organically ever since.

@mitchcapper
Copy link
Contributor Author

I am not 100% sure what you are mentioning. Maybe you are suggesting changes where I am hooking up handlers and calling click events from the code behind? I could hard code the key down/preview key down events in the designer class on each item separately just seemed liked replicating code that might be better in one spot. I can probably move the function that loops through the controls into the design file itself, although I think if a designer file had anything drastically different form the automatically generated design code I would not expect that.

As for calling the performclick rather than just the end logic itself, my goal was to provide visual feedback to the user. Granted I didn't spend any time to see if performclick actually did whatever the click style changes were.

I really know pretty little about the modern WinForms design patterns to separate the designer and code logic (beyond just the basics of what already exists with the .designer.cs and standard code behind file). Plenty of xaml with views and models but really no idea how to do a decent split here. I don't think there is such separation already in the code to follow the pattern on?

I am not overly attached to the code either, just coded it to investigate my use case and did some basic cleanup to make it potentially useful to others.

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label May 2, 2023
@davidwrighton
Copy link
Member

I've fixed the code formatting issues, and this is on track to checkin today/tomorrow

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-crossgen2-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants