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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow diff viewer to be expanded #844

Merged
merged 2 commits into from Jun 17, 2023

Conversation

micahmo
Copy link

@micahmo micahmo commented Jun 8, 2023

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • New feature or enhancement
  • UI change (please include screenshot!)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Internationalization and localization
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

I have a small change to propose. I thought it would be nice to create a mechanism whereby various code editors could be "expanded". In particular, I was finding that the diff editor is very small, and I am often comparing large files.

I have added a DependencyProperty to the code editor allowing it to display an expand/collapse button. Pressing the button triggers an event that the parent control can handle. From there, the UI can be adjusted such that the editor is larger than normal.

Once the mechanism is in place, other tool pages can support expandable code editors using the same mechanism, following these steps.

  • In XAML
    • Set AllowExpand="True" on the CodeEditor and subscribe to ExpandedChanged.
    • Create a placeholder control into which the CodeEditor can be placed to fill the whole page.
  • In code-behind
    • When the CodeEditor is expanded, move it into the placeholder control. Optionally hide other controls.
    • When the CodeEditor is collapsed, move it back to its original control. Optionally show other controls.

I know this is not really nice MVVM, but (a) CodeEditor doesn't have a ViewModel anyway and (b) since this is purely UI manipulation, I don't mind doing it in the code-behind. If this is not good architecture (or if you don't like this feature), of course there is no obligation to accept! 馃槉

NOTE: It is recommended to review this PR with "Hide whitespace" selected so that the changes in TextDiffToolPage.xaml do not look as big.

Other information

DevToys-Expand-Demo.mp4

Quality check

Before creating this PR:

  • Did you follow the code style guideline as described in CONTRIBUTING.md
  • Did you build the app and test your changes?
  • Did you check for accessibility? On Windows, you can use Accessibility Insights for this.
  • Did you verify that the change work in Release build configuration
  • Did you verify that all unit tests pass
  • If necessary and if possible, did you verify your changes on:
    • Windows
    • macOS (DevToys 2.0)
    • Linux (DevToys 2.0)

@veler veler linked an issue Jun 8, 2023 that may be closed by this pull request
@veler
Copy link
Collaborator

veler commented Jun 11, 2023

Hello,
Sorry for the late review on this one, and thank you very much for this PR!

This is a great change that would indeed improve the experience of Text Diff tool. That being said, we also got feedback from customers that it would be nice to do this for formatters, like JSON or SQL ones.

I wonder if it would be possible to find a more generic approach to this problem. For example, creating a reusable control that would accept any control on the left, any control on the right, have a grid splitter in the middle, and allow to expand the right side so it takes the whole space. This could be reused in many tools like:

  • JSON <> YAML Converter
  • HTML, URL, Base64 Text, GZip Encoder / Decoder
  • JSON, SQL and XML Formatter
  • Text Escape / Unescape
  • Text Diff (indeed)

What do you think?

PS: I linked an issue to this PR. I was planning to address this feedback in DevToys 2.0. Happy to think about how the look and feel should look like beforehand though.

@micahmo
Copy link
Author

micahmo commented Jun 12, 2023

Hi @veler, no worries on the late reply, I know you're busy.

I totally agree that it would be useful for many more text/code areas to be expandable. Personally, I would like to expand the JWT payload control.

creating a reusable control that would accept any control on the left, any control on the right, have a grid splitter in the middle, and allow to expand the right side so it takes the whole space

I'm not sure this is quite as generic as we need.

  • It wouldn't work for the Text Comparer, since the controls are split horizontally, not vertically (although I suppose the split direction could be a parameter).
  • It's not that much better than just a grid splitter, which can already almost occupy the whole space with one side.
  • It wouldn't work well for the JWT page, which has other (often large) text input/output fields.
  • It wouldn't allow the control to take the space of the configuration options at the top (for example, there are some tool pages with up to three uncollapsed options).

After trying lots of things, I picked the proposed approach because I thought it did offer some amount of reusability.

  • The code editor itself owns the button and the event, so it can be turned on anywhere.
  • However, the owning page must subscribe to the event and decide how it affects the layout of the page. This allows it to hide and replace any other control with the one that is requesting focus.

I believe it would only require work in two files (like here and here) to enable elsewhere.

I can think of another approach, which is a generic page wrapper. The wrapper would contain...

  • An area for expanded controls
  • An area holding the "normal" page

It could dynamically detect controls potentially requesting focus (e.g., traversing visual tree with reflection, just an idea), and when the event is triggered, it could bring the requested control into focus and hide the normal page. I would have to experiment with this since it wouldn't be the direct parent of the control that it is manipulating. But it could also allow this procedure to happen much more generically simply by enabling AllowExpand on a code editor.

What do you think?

@veler
Copy link
Collaborator

veler commented Jun 14, 2023

Hello,

After re-reading the code, you're right, it's already quite generic and making it even more generic may complexify a lot.

Would it be possible to give a try at extending this to the following tools?

  • JSON <> YAML Converter
  • HTML, URL, Base64 Text, GZip Encoder / Decoder
  • JSON, SQL and XML Formatter
  • Text Escape / Unescape

@micahmo
Copy link
Author

micahmo commented Jun 14, 2023

Would it be possible to give a try at extending this to the following tools?

Of course! I think that's a great idea and would test whether it's really as generic/easy as I'm hoping. 馃槉

@micahmo
Copy link
Author

micahmo commented Jun 14, 2023

Alrighty, I have updated the PR with expandable editors for the tools you requested (plus the JWT tool). It turns out the technique works pretty well. 馃槉 BTW, in all cases I only made the output control expandable (with the exception of JWT, where I did payload, which is the input for encode and output for decode). But it would be just as easy to add this to any CustomTextBox or CodeEditor in the application.

DevToys-Expandable-Demo.mp4

@veler
Copy link
Collaborator

veler commented Jun 17, 2023

Hello,
Sorry for the delay to review this PR. It looks perfect to me! :D Approved! Thank you a thousand times for all these amazing contributions!

@veler veler merged commit 27429f4 into DevToys-app:main Jun 17, 2023
1 check passed
@micahmo micahmo deleted the feature/expandable-editor branch June 17, 2023 14:35
@veler
Copy link
Collaborator

veler commented Jul 2, 2023

Here is how it looks on DevToys 2.0 馃グ (a bit buggy at the moment)

Recording.2023-07-02.122847.mp4

@micahmo
Copy link
Author

micahmo commented Jul 3, 2023

Here is how it looks on DevToys 2.0 馃グ

Looks wonderful, nice work!

I'm assuming you are and will be working on the framework for a while, and then when it's ready, you'll start porting tools?

@veler
Copy link
Collaborator

veler commented Jul 3, 2023

Here is how it looks on DevToys 2.0 馃グ

Looks wonderful, nice work!

I'm assuming you are and will be working on the framework for a while, and then when it's ready, you'll start porting tools?

Exactly! That's why we see dummies controls in the settings page at the moment 馃槄

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

Successfully merging this pull request may close these issues.

Can Text compare result area to be full windowed?
2 participants