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

Show non-printable characters #13020

Closed
wants to merge 9 commits into from

Conversation

ozone10
Copy link
Contributor

@ozone10 ozone10 commented Feb 1, 2023

fix issue with tooltips in preference dialog, when changing between dark/light mode

image

defined for:

Codepoint : character : name : abbreviation
U+00A0 :   : no-break space : NBSP
U+1680 :   : ogham space mark : OSPM
U+180E : ᠎ : mongolian vowel separator : MVS
U+2000 :   : en quad : NQSP
U+2001 :   : em quad : MQSP
U+2002 :   : en space : ENSP
U+2003 :   : em space : EMSP
U+2004 :   : three-per-em space : 3/MSP
U+2005 :   : four-per-em space : 4/MSP
U+2006 :   : six-per-em space : 6/MSP
U+2007 :   : figure space : FSP
U+2008 :   : punctation space : PSP
U+2009 :   : thin space : THSP
U+200A :   : hair space : HSP
U+200B : ​ : zero-width space : ZWSP
U+200C : ‌ : zero-width non-joiner : ZWNJ
U+200D : ‍ : zero-width joiner : ZWJ
U+200E : ‎ : left-to-right mark : LRM
U+200F : ‏ : right-to-left mark : RLM
U+2028 : 
 : line separator : LS
U+2029 : 
 : paragraph separator : PS
U+202A : ‪ : left-to-right embedding : LRE
U+202B : ‫ : right-to-left embedding : RLE
U+202C : ‬ : pop directional formatting : PDF
U+202D : ‭ : left-to-right override : LRO
U+202E : ‮ : right-to-left override : RLO
U+202F :   : narrow no-break space : NNBSP
U+205F :   : medium mathematical space : MMSP
U+2060 : ⁠ : word joiner : WJ
U+2066 : ⁦ : left-to-right isolate : LRI
U+2067 : ⁧ : right-to-left isolate : RLI
U+2068 : ⁨ : first strong isolate : FSI
U+2069 : ⁩ : pop directional isolate : PDI
U+206A :  : inhibit symmetric swapping : ISS
U+206B :  : activate symmetric swapping : ASS
U+206C :  : inhibit arabic form shaping : IAFS
U+206D :  : activate arabic form shaping : AAFS
U+206E :  : national digit shapes : NADS
U+206F :  : nominal digit shapes : NODS
U+3000 :   : ideographic space : IDSP
U+FEFF :  : zero-width no-break space : ZWNBSP

fix #827
fix #4731
fix #8284

fix issue with tooltips in preference dialog, when changing between dark/light mode
@pryrt
Copy link
Contributor

pryrt commented Feb 1, 2023

From a UI perspective -- I originally thought those 1, 2, and ? were supposed to be buttons of some sort, and tried clicking them rather than just hovering. And this was even having seen the "fix issue with tooltips" in the PR. So I'm not sure that your average user is going to know to hover rather than click on those. Is there a way to make clicking the box also pop up the hover text?

Also, how critical is it, really, to have that full list in the UI? Would it be better to just put those lists in the User Manual, and just have the Preferences dialog show the Abbreviation and Codepoint radio buttons, without the 1,2,? non-buttons. Maybe have a short hover for each, so Abbreviation has a hover of e.g. NBSP for the no-break space character; see the User Manual for the full list, and Codepoint has a hover of e.g. U+00A0 for the no-break-space character; see the User Manual for the full list.

@alankilborn
Copy link
Contributor

Here's someone else's definition of "non-printable": #8111

@alankilborn
Copy link
Contributor

I originally thought those 1, 2, and ? were supposed to be buttons of some sort

I endorse @pryrt 's feelings about these buttons, including "is it really needed".

If user clicks one, nothing happens. Worse, after the click attempt, user's mouse points to the button while user waits for something to happen. This causes the tip to NEVER appear (without user moving mouse away and then back into button space).

@Yaron10
Copy link

Yaron10 commented Feb 1, 2023

@ozone10,

Thank you for your work. 👍

Toggling the command or changing the preference does not take effect until clicking in the editing area.
Can this be changed?

@Yaron10
Copy link

Yaron10 commented Feb 1, 2023

Would it be better to just put those lists in the User Manual, and just have the Preferences dialog show the Abbreviation and Codepoint radio buttons, without the 1,2,? non-buttons.

👍

A link to the User Manual is another option.

@ozone10
Copy link
Contributor Author

ozone10 commented Feb 1, 2023

@pryrt, @alankilborn,
thx for feedback.
I did some quick tweaks for GUI could you check it.

@Yaron10

Toggling the command or changing the preference does not take effect until clicking in the editing area. Can this be changed?

I've noticed it too, but I am not sure how to fix it now.

For [?] button I've just copied code. The same issues also affect performance and delimiter [?] buttons.

@Yaron10
Copy link

Yaron10 commented Feb 1, 2023

@ozone10,

I've noticed it too, but I am not sure how to fix it now.

A known-issue is legit. :)

@ozone10
Copy link
Contributor Author

ozone10 commented Feb 1, 2023

@alankilborn

Here's someone else's definition of "non-printable": #8111

well this PR is more about "non-ASCII" non-printable characters.

@pryrt
Copy link
Contributor

pryrt commented Feb 1, 2023

The same issues also affect performance and delimiter [?] buttons.

Yes, well, I've never liked those buttons' behavior, either. I've just never felt strongly enough about the other two to put in a separate issue, since I only used those two hover texts once (when trying to figure out what the button was supposed to do). For your button, I was just in the loop early enough to be able to complain about it before it was released ;-)

I did some quick tweaks for GUI could you check it.

I downloaded the x64 release build exe from appveyor, and I definitely prefer the updated Preferences page: the single ? works more as expected: a slow mouse user will see a hover text telling them to click for more details, and a fast mouse user will click that button before the hover and still get all the info. And thanks for adding hover text to the two radio button labels as well. So that commit is a definite improvement on the Preferences side.

@pryrt pryrt mentioned this pull request Feb 1, 2023
@alankilborn
Copy link
Contributor

A link to the User Manual is another option.

There's already a link to the user manual in the ? menu.
But I think you mean a (future) direct link to the topic at hand.
The problem with that is that then there becomes a maintenance issue as that link has to live forever.
And someone has to remember that it can't ever be changed.
I'm not saying I'm opposed, but it would be a great disappointment if someday such a link stops working.

@alankilborn
Copy link
Contributor

Here's someone else's definition of "non-printable": #8111

well this PR is more about "non-ASCII" non-printable characters.

Hmm, why not make a "complete" solution? Not that I know what that looks like... :-)

@alankilborn
Copy link
Contributor

alankilborn commented Feb 1, 2023

I really don't think Show All Characters is the correct name for showing whitespace+line-endings any longer, with the advent of the new command put forth by this PR. Could this PR adjust that, or would that be a subject for a future issue/PR?

Probably Show White Space and TAB is poorly named as well. As this is done in a different color rather than reverse video like EOL and these new "non-printable" characters, maybe Emphasize U+0020 Whitespace and TAB Characters is better? Or Emphasize Whitespace and TAB Characters? (but that loses something without the U+0020, sigh).

@pryrt
Copy link
Contributor

pryrt commented Feb 1, 2023

I don't know how I feel about Emphasize. The fact that it's not a reverse-video symbol doesn't change the fact that it is still showing a symbol for the space and tab characters: it might not be the same kind of symbol, but it's still a symbol, and the option does toggle whether or not that symbol is shown.

But more than that, Whitespace and TAB Characters was never an accurate description: "TAB" in all-caps is a holdover from people trying to make the tab character name look like the other ASCII control character abbreviations, though it's official abbreviation is actually HT. (Though I understand that the TAB notation is used throughout the UI, so some might argue it should stay TAB here for consistency, since this PR isn't going to change TAB to Tab throughout the UI. But I'd vote to use Tab here [first-letter capitalized, to match the capitalization of Space here and the capitalization of most other nouns and adjectives in the menu system, and then maybe put in a separate issue/PR to make Tab the consistent usage throughout the UI.)

And the tab character is a Whitespace character, as are newlines and vertical tabs and some of the "non-printable" Unicode characters, so separating "Tab" as somehow distinct from the "Whitespace" or "White Space" is not an accurate separation. Since the only two characters affected by that toggle are the literal space (ASCII 32, U+0020) and the tab (ASCII 9, U+0009), I think that Show Space and Tab is more accurate and understandable.

As for the new name for the never-accurate Show All Characters, how about Show Space, Tab, and End of Line -- that is, just merge the names of the two previous menu entries into the third, multi-use entry.

@ozone10
Copy link
Contributor Author

ozone10 commented Feb 1, 2023

@alankilborn

Hmm, why not make a "complete" solution? Not that I know what that looks like... :-)

Because ASCII characters are already visible. This PR is dealing with "invisible" characters.

IMHO I think Show White Space and TAB should be Show Space and Tab and Show All Characters can stay as it currently also enable/disable Show Non-Printable Characters

@pryrt
Copy link
Contributor

pryrt commented Feb 1, 2023

Show All Characters can stay as it currently also enable/disable Show Non-Printable Characters

Oh, I hadn't noticed that toggling the Show All also forced the new menu entry to toggle.

I'm not sure I like that, because then to move from a current state of Show Space and Tab to the state where it shows space, tab, and newlines, would require first clicking on Show All Characters and then un-clicking Show Non-Printable Characters. For people (like me) who are used to quickly toggling between their two favorite "show symbol" states, requiring us to go back to the menu a second time to change to the state we actually intended would get annoying really quickly.

@alankilborn
Copy link
Contributor

Hmm, why not make a "complete" solution? Not that I know what that looks like... :-)

Because ASCII characters are already visible. This PR is dealing with "invisible" characters.

Sure, OK, in the spirit of keeping changes small. Hopefully if someone does a solution for not showing the ASCII characters, it fits in well with this PR's changes.

@alankilborn
Copy link
Contributor

Oh, I hadn't noticed that toggling the Show All also forced the new menu entry to toggle.

I hadn't noticed that either.
I guess we had to read the code in order to understand the nuances of how this PR works.

@alankilborn
Copy link
Contributor

@ozone10,
Thank you for your work. 👍

This is true. Praise where praise is due.
NICE WORK, @ozone10 !
I hope this gets adopted, perhaps with some slight tweaks.
Again, great job.

@chcg chcg added the feature Feature requests and Feature commits label Feb 1, 2023
@Yaron10
Copy link

Yaron10 commented Feb 2, 2023

There's already a link to the user manual in the ? menu.
But I think you mean a (future) direct link to the topic at hand.
The problem with that is that then there becomes a maintenance issue as that link has to live forever.

👍

Oh, I hadn't noticed that toggling the Show All also forced the new menu entry to toggle.
I'm not sure I like that...

👍

pnedev/comparePlus#345 (comment).

See how NPP implements "View -> Show Symbol -> Show White Space and Tab" and "View -> Show Symbol -> Show End of Line".
Checking one command, automatically unchecks the other.
IMO, that behavior is wrong in that case: the two commands are not contradicting whatsoever.
(The third command "Show All Characters" probably made the coder implement it that way).


@ozone10,

Show White Space and TAB and Show End of Line are currently (in Master) Show White Space and TAB ONLY and Show End of Line ONLY.
If you keep the linkage between Show Non-Printable to Show All Characters, checking either Show White Space and TAB or Show End of Line should uncheck Show Non-Printable.

(IMO, Show All Characters should be removed and each command should be independent).

If you use a MsgBox, how about replacing the ? with ...?

I also think that Non-Printable is not the best term.

Thanks again.

@alankilborn
Copy link
Contributor

Show All Characters should be removed and each command should be independent

I agree, this would be better than existing implementations, both as currently in released N++, and in this PR.

If you use a MsgBox, how about replacing the ? with ...?

Makes sense.

I also think that Non-Printable is not the best term.

Agree, but your lack of suggesting a substitute shows how difficult coming up with a good substitute is. :-)
Someone else suggested to me "Non-glyphed", which is growing on me...

@ozone10
Copy link
Contributor Author

ozone10 commented Feb 2, 2023

Sorry wrong button.

@pryrt

I'm not sure I like that, because then to move from a current state of Show Space and Tab to the state where it shows space, tab, and newlines, would require first clicking on Show All Characters and then un-clicking Show Non-Printable Characters. For people (like me) who are used to quickly toggling between their two favorite "show symbol" states, requiring us to go back to the menu a second time to change to the state we actually intended would get annoying really quickly.

image

@alankilborn, @Yaron10

Should it be "non-printing or control and whitespace characters"?

https://en.wikipedia.org/wiki/Control_character
https://en.wikipedia.org/wiki/Whitespace_character

@Yaron10
Copy link

Yaron10 commented Feb 4, 2023

@ozone10,

May I ask you when npcSync should be no?

Thank you.

@pryrt
Copy link
Contributor

pryrt commented Feb 4, 2023

@pryrt
In which section we should put this information? Could you provide the URL please?

@donho , it will be in the https://npp-user-manual.org/docs/views/#show-symbol section

pryrt added a commit to pryrt/npp-usermanual that referenced this pull request Feb 4, 2023
New feature in active development per notepad-plus-plus/notepad-plus-plus#13020

Started the documentation now, even though the feature hasn't been committed, because Don wanted to know what the URL would be for the table.

The table was populated based on @ozone10's list that was previously shown in dialog box, but with the abbreviations updated based on https://en.wikipedia.org/wiki/List_of_Unicode_characters#General_Punctuation

The feature is still in-progress, so the documentation is, too.
@donho
Copy link
Member

donho commented Feb 4, 2023

@Yaron10

May I ask you when npcSync should be no?

Well caught.
npcSync is initialized as yes if this argument is absent. It will never be modified except user changes it manually in config.xml.
I suppose it was for GUI "sync with show all" option initially but it's forgotten to be deleted when the GUI option was removed.

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.

  1. Please follow the suggestions.
  2. Add NPC custom color node in bool NppParameters::feedStylerArray(TiXmlNode *node) similar as EOL custom color node (so "NPC custom color" node will be always available, even the styler.xml is not updated):
    https://github.com/notepad-plus-plus/notepad-plus-plus/blob/master/PowerEditor/src/Parameters.cpp#L3886

PowerEditor/src/WinControls/ColourPicker/WordStyleDlg.cpp Outdated Show resolved Hide resolved
PowerEditor/src/WinControls/Preference/preferenceDlg.cpp Outdated Show resolved Hide resolved
PowerEditor/src/Parameters.h Outdated Show resolved Hide resolved
handle missing NPC color style in theme files,
open User Manual directly to relevant page,
tweaks
@ozone10
Copy link
Contributor Author

ozone10 commented Feb 4, 2023

@Yaron10

May I ask you when npcSync should be no?

It was supposed to be hidden option for users who don't want change how Show All Characters currently works (show/hide space, tab and EOL).

But unless there is demand for it I have removed it for now.

@donho

So that's the table we should put in https://npp-user-manual.org/ Do you confirm? If it's, could you provide the code in Markdown or HTML please?

This was only for reference. This PR also include other non-printing and whitespace characters.
Table should be based on #13020 (comment)

@ozone10
Copy link
Contributor Author

ozone10 commented Feb 4, 2023

@donho

Name Codepoint Abbreviation
No-Break Space U+00A0 NBSP
Ogham Space Mark U+1680 OSPM
Mongolian Vowel Separator U+180E MVS
En Quad U+2000 NQSP
Em Quad U+2001 MQSP
En Space U+2002 ENSP
Em Space U+2003 EMSP
Three-Per-Em Space U+2004 3/MSP
Four-Per-Em Space U+2005 4/MSP
Six-Per-Em Space U+2006 6/MSP
Figure Space U+2007 FSP
Punctation Space U+2008 PSP
Thin Space U+2009 THSP
Hair Space U+200A HSP
Zero-Width Space U+200B ZWSP
Zero-Width Non-Joiner U+200C ZWNJ
Zero-Width Joiner U+200D ZWJ
Left-To-Right Mark U+200E LRM
Right-To-Left Mark U+200F RLM
Line Separator U+2028 LS
Paragraph Separator U+2029 PS
Left-To-Right Embedding U+202A LRE
Right-To-Left Embedding U+202B RLE
Pop Directional Formatting U+202C PDF
Left-To-Right Override U+202D LRO
Right-To-Left Override U+202E RLO
Narrow No-Break Space U+202F NNBSP
Medium Mathematical Space U+205F MMSP
Word Joiner U+2060 WJ
Left-To-Right Isolate U+2066 LRI
Right-To-Left Isolate U+2067 RLI
First Strong Isolate U+2068 FSI
Pop Directional Isolate U+2069 PDI
Inhibit Symmetric Swapping U+206A ISS
Activate Symmetric Swapping U+206B ASS
Inhibit Arabic Form Shaping U+206C IAFS
Activate Arabic Form Shaping U+206D AAFS
National Digit Shapes U+206E NADS
Nominal Digit Shapes U+206F NODS
Ideographic Space U+3000 IDSP
Zero-Width No-Break Space U+FEFF ZWNBSP

@ozone10 ozone10 requested a review from donho February 4, 2023 16:25
@donho
Copy link
Member

donho commented Feb 4, 2023

@ozone10
[xml] Update default themes is failed due to [xml]:
https://ci.appveyor.com/project/donho/notepad-plus-plus/builds/46140254

Could you do a new force push please ?

@Yaron10
Copy link

Yaron10 commented Feb 4, 2023

@donho & @ozone10,

npcSync is initialized as yes...

It was supposed to be hidden option...

Thank you both. 👍

@ozone10
Copy link
Contributor Author

ozone10 commented Feb 4, 2023

failed due to [xml]

Could you do a new force push please ?

I thought commit with changes only to xml should use [xml] in commit name.

@donho donho closed this in aaab190 Feb 5, 2023
@ozone10 ozone10 deleted the NonPrintChars branch February 5, 2023 08:22
@alankilborn
Copy link
Contributor

Can this be made the default setting?:

image

This is such a valuable new feature, but if it is not on-by-default, 90+% of Notepad++ users won't ever know about it and won't be able to benefit from it (the typical user upgrades all the time, but doesn't read release notes).

Make it on by default, let those that complain figure out or ask how to turn it off (if they want, although turning it off is usually not a smart move).

@alankilborn
Copy link
Contributor

@ozone10 @donho

When Show Non-Printing characters is enabled, how should they appear in Search results?

image

@donho
Copy link
Member

donho commented Feb 14, 2023

Hmm... Maybe it's better that Search result' Scintilla sync with settings (only this part).
@alankilborn Could you create an issue for that please?

@alankilborn
Copy link
Contributor

Could you create an issue for that please?

@ozone10 's PR made it in before the issue could be created.

pryrt added a commit to notepad-plus-plus/npp-usermanual that referenced this pull request Feb 27, 2023
* Show Non-Printable Characters

New feature in active development per notepad-plus-plus/notepad-plus-plus#13020

Started the documentation now, even though the feature hasn't been committed, because Don wanted to know what the URL would be for the table.

The table was populated based on @ozone10's list that was previously shown in dialog box, but with the abbreviations updated based on https://en.wikipedia.org/wiki/List_of_Unicode_characters#General_Punctuation

The feature is still in-progress, so the documentation is, too.

* Non-Printing Chars: make the table collapsible

* update Table of Non-Printing Characters

regenerate table from https://github.com/notepad-plus-plus/notepad-plus-plus/pull/13063/files

* restore table header

accidentally deleted the header in the previous commit

* added v8.5 and clarified
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature requests and Feature commits
Projects
None yet
6 participants