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.

Sign up for GitHub

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

Jump to bottom

Replace tab size link mini dlg with edit control #13177

Closed
wants to merge 3 commits into from
Closed

Replace tab size link mini dlg with edit control #13177

wants to merge 3 commits into from

Conversation

ozone10
Copy link
Contributor

@ozone10 ozone10 commented Feb 19, 2023

image

fix #11695
fix #13176

@chcg chcg added the enhancement Proposed enhancements of existing features label Feb 19, 2023
@Yaron10
Copy link

Yaron10 commented Feb 19, 2023

@ozone10,

👍
Thank you for this too. Two more. :)

  1. If the user enters 0 (zero) in the EditBox and moves focus to another element, that value is not auto-changed.
  2. Replace with Spaces might still be blurry in DarkMode .

@Yaron10
Copy link

Yaron10 commented Feb 20, 2023

@ozone10,

Thank you for the quick fix. 👍

How about implementing it in if (tabSize < 1) under case EN_CHANGE:?
That is: emulate Windows behavior on trying to enter non-numeric chars, and if the users enters 0 - beep and restore the previous value.
(The user won't be able to type 01, but that shouldn't be a problem IMO).

@ozone10
Copy link
Contributor Author

ozone10 commented Feb 20, 2023

@Yaron10

  1. Replace with Spaces might still be blurry in DarkMode .

That is checkbox button text, and use different text drawing for disabled state than static text, if there was issue with it, it should also affect other places.

How about implementing it in if (tabSize < 1) under case EN_CHANGE:? That is: emulate Windows behavior on trying to enter non-numeric chars, and if the users enters 0 - beep and restore the previous value. (The user won't be able to type 01, but that shouldn't be a problem IMO).

Currently there is no difference between 0 and empty value. Differentiating would make code more complex.
Also consider value e.g. 10 user will put caret before 1 press Delete -> value will change to 0 -> value will be restored to 10. This is not good.

@Yaron10
Copy link

Yaron10 commented Feb 20, 2023

@ozone10,

Currently there is no difference between 0 and empty value.
...
Also consider value e.g. 10 user will put caret before 1...

Both arguments are very good. Thank you for the explanation. 👍

תמונה
תמונה

If you can't reproduce it, it may be caused by my Windows 10 settings and custom theme.
You know I don't use DarkMode. :)

@ozone10 ozone10 mentioned this pull request Feb 21, 2023
Closed
@donho donho added the accepted label Feb 21, 2023
@sXNA sXNA mentioned this pull request Feb 22, 2023
Closed
@donho donho assigned donho and unassigned ozone10 Feb 23, 2023
donho
donho requested changes Feb 23, 2023
View reviewed changes
Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

redrawDlgItem uses MapWindowPoints function which converts (maps) a set of points from a coordinate space relative to one window to a coordinate space relative to another window, then redraw it with the new coordonate points.
I don't see the reason for it.
Please provide the arguments for this new added method.

PowerEditor/src/WinControls/Window.h Outdated
@@ -64,8 +64,20 @@ class Window
::UpdateWindow(_hSelf);
}

virtual void redrawDlgItem(const int nIDDlgItem, bool forceUpdate = false) const
Copy link
Member

Choose a reason for hiding this comment

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

What does it do exactly this function?
And why do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@donho
currently using redraw() will redraw all items in window which is unnecessary and can cause flickering.
Try switching fast options in Preference->Editing->Current Line Indicator.

MapWindowPoints is used to get correct coordinates of child rect for redrawing.
https://stackoverflow.com/questions/18034975/how-do-i-find-position-of-a-win32-control-window-relative-to-its-parent-window

Copy link
Member

Choose a reason for hiding this comment

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

@ozone10
In this case, this method should be in class StaticDialog instead of class Window.

@ozone10 ozone10 requested a review from donho February 23, 2023 17:30
donho
donho requested changes Feb 23, 2023
View reviewed changes
Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

Please put the method redrawDlgItem in class StaticDialog instead of class Window.

PowerEditor/src/WinControls/Window.h Outdated
@@ -64,8 +64,20 @@ class Window
::UpdateWindow(_hSelf);
}

virtual void redrawDlgItem(const int nIDDlgItem, bool forceUpdate = false) const
Copy link
Member

Choose a reason for hiding this comment

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

@ozone10
In this case, this method should be in class StaticDialog instead of class Window.

@donho donho closed this in 8b3f072 Feb 24, 2023
@ozone10 ozone10 deleted the PrefDlgMiniDlgTabSize branch February 24, 2023 06:25
@Yaron10
Copy link

Yaron10 commented Feb 24, 2023

Fixes also #12558.
👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@donho donho donho requested changes

Assignees

@donho donho

Labels
accepted enhancement Proposed enhancements of existing features
Projects
None yet
Milestone
No milestone
4 participants
@ozone10 @Yaron10 @donho @chcg