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

Style Token not applied consistently within words #9751

Closed
juvtib opened this issue Apr 9, 2021 · 42 comments
Closed

Style Token not applied consistently within words #9751

juvtib opened this issue Apr 9, 2021 · 42 comments
Assignees

Comments

@juvtib
Copy link

juvtib commented Apr 9, 2021

I noticed an issue with the "style token" feature. The problem is that styling is not applied consistently within words.

Case 1

  1. Close and open Notepad++.
  2. Type "training" on line 1 and "train" on line 2.
  3. With nothing selected, right-click on "train" on line 2 > Style token > Using 1st Style.

Under these conditions, only "train" on line 2 is styled.

screen capture where line 2 is styled


Case 2

  1. Close and open Notepad++.
  2. Type my test: "training" on line 1 and "train" on line 2.
  3. Open the Find dialog.
  4. Clear 'Match whole word only'.
  5. With nothing selected, right-click on "train" on line 2 > Style token > Using 1st Style.

Under these conditions, "train" on lines 1 and 2 are styled: The token will be styled within words.

screen capture where both lines are styled

Style Token will continue to match within words after opening the Find dialog once.

Expected Behavior

I am accustomed to case 2 where style applies to matches within words. This is how Notepad++ operates most of the time.

It doesn't make sense that a Find dialog option would affect Style Token. I suggest decoupling Style Token from the Find dialog. If people want to control this behavior then I suggest adding a formal preference: Style Token Matching.

Debug Information

Notepad++ v7.9.5 (32-bit)
Build time : Mar 21 2021 - 02:09:07
Path : C:\Program Files (x86)\Notepad++\notepad++.exe
Admin mode : OFF
Local Conf mode : OFF
OS Name : Windows 10 Enterprise (64-bit)
OS Version : 2009
OS Build : 19042.867
Current ANSI codepage : 1252
Plugins : mimeTools.dll NppConverter.dll NppExport.dll

@juvtib
Copy link
Author

juvtib commented Apr 9, 2021

This might be related to #5148

@sasumner
Copy link
Contributor

sasumner commented Apr 9, 2021

Discussed in the Community in the 7.9.5 release announcement thread, starting with this posting: https://community.notepad-plus-plus.org/post/64738

@sasumner sasumner self-assigned this Apr 9, 2021
@sasumner
Copy link
Contributor

sasumner commented Apr 9, 2021

Note: "Style Token" > Using 1st Style is not an official name for the command. It (the "Style Token" part) is a right-click context menu folder grouping name.

The command is actually found in the Search menu, submenu Mark All, then Using 1st Style.

@sasumner
Copy link
Contributor

sasumner commented Apr 10, 2021

It doesn't make sense that a Find dialog option would affect Style Token. I suggest decoupling Style Token from the Find dialog. If people want to control this behavior then I suggest adding a formal preference.

It is my idea to improve the situation described by this issue by adding the following:

image

Only possible to have one item checked.

The "classic" setting preserves existing behavior.

I wasn't sure the best way to express "partial" word matching, because of course a "whole" word matches also when "partial" is in effect. So I expressed it as "partial/whole"...there may be a better way to express it.

@donho and others, what are the thoughts about it?

@sasumner
Copy link
Contributor

The idea behind "classic" settings was to retain how settings for the feature currently (7.9.5) work...but...there is a lot of overlap between classic and "use Find dialog" settings. Not total overlap however. My feeling is that probably "classic" should be dropped as a choice, but then there would be no retention of existing behavior. Of course, the existing behavior is a bit confusing for users...

@juvtib
Copy link
Author

juvtib commented Apr 13, 2021

Creating a sub-menu makes the solution too complicated for users. Features in the context menu, such as Style Token, should be simple.

The main issues I reported are that the matching behavior is inconsistent and using Find dialog settings makes no sense to someone right-clicking on a word.

If someone breaks the link to the Find dialog settings, they'll have to choose a matching algorithm. I suggested matching within words.

As far as I'm concerned, breaking the link to Find and matching within words is a complete solution.


It seems like Mark All and Find are sharing code. I would look for a way to make the parameters relating to Find optional. Then Mark All and Find can share without affecting each other. I come from a discipline where you would pass those parameters explicitly. Instead, maybe the optional parameter can be whether to use the Find state or a hard coded state.

It might be worth naming these features consistently if Search > Mark All and Style Token are the same.

I found this article helpful:

@sasumner
Copy link
Contributor

sasumner commented Apr 13, 2021

@juvtib

Thank you for your thoughts.

Creating a sub-menu makes the solution too complicated for users.

The suggested submenu is a "settings" area; users can ignore it (their choice) but for those that want extra configuration power, they will have that capability. It is probably not something that is changed before every "Mark All" operation; users will have their preferred way of working and probably set it one time and then forget about it.

Features in the context menu, such as Style Token, should be simple.

I attempted to explain earlier, but the existing "Mark all" feature has more capability than what "Style Token" in the context menu implies.

The main issues I reported are that the matching behavior is inconsistent and using Find dialog settings makes no sense to someone right-clicking on a word.

And my addressing the issue acknowledges that.

It seems like Mark All and Find are sharing code.

The only sharing relevant to the discussion here is that currently Mark All grabs the case and whole word settings from the UI choices on the Find window, in certain circumstances.

If someone breaks the link to the Find dialog settings, they'll have to choose a matching algorithm. I suggested matching within words.
As far as I'm concerned, breaking the link to Find and matching within words is a complete solution.

Probably a good solution for the way you use it; maybe not good for everyone.

It might be worth naming these features consistently if Search > Mark All and Style Token are the same.

Apparently the author of Notepad++ likes the Style Token nomenclature in the context menu.

"when asked to fix something, we don't even think of removing parts"

The reason that this is often the case is that if you remove parts, then something that has worked perfectly for N number of users for years is now broken. Now, you may think it is broken as it is, but I'm sure others would disagree, perhaps vehemently so.

@donho
Copy link
Member

donho commented Apr 15, 2021

@sasumner
IMO there's no place on menu command for any settings, including "Mark All settings".

@ArkadiuszMichalski
Copy link
Contributor

@sasumner
Does it only affect Mark All? If so, not better to put it in Mark All menu, after the separator? It would be in one place, immediately available in the context menu.

image

Although the above comment is not optimistic anyway to solve it in this way.

@sasumner
Copy link
Contributor

Does it only affect Mark All?

Yes. It makes no sense for "Mark One".
It is actually a great idea to put it where you show! :-)

@sasumner
Copy link
Contributor

sasumner commented Apr 15, 2021

IMO there's no place on menu command for any settings, including "Mark All settings".

So the existing "mark with style" commands are supposed to be "lightweight" and not heavily UI involved.
Meaning that they don't have something like a Find window tab (where choices could be set) -- but rather are run with a keycombo (I would guess that is typical user use-case, I know it is mine).

To keep with the spirit of lightweight UI, I came up with the design I did.
These could be Preference settings, but I felt that was a bit burdensome to have to slog through the Preferences UI in order to set/change them, because certain users (like me!) might want to change them more than once.

Certainly we have precedent for "settings" within menus, here's one for example:

image

I really wish people would have commented on the core idea presented in the issue, BEFORE I decided to go to the trouble to code it and prepare a PR.
I took a lack of response in the issue comments as being tacit approval, or certainly no real objection on the approach.
It is a shame to have to wait for PR time for people to start offering opinions. :-(
I'm talking about frequent contributors, not necessarily the opinions of generic users (who may not know "best").

It's acknowledged that sometimes people trying out the exe from the PR generates additional commentary -- that is certainly acceptable -- but really isn't the case here.

@donho
Copy link
Member

donho commented Apr 15, 2021

@sasumner

Certainly we have precedent for "settings" within menus, here's one for example:

Yes, I know this one. It is an acceptable exception because

  1. it's in Find in Files zone - easier to find for users who use FiF frequently (than in Preferences dialog).
  2. users may need to enable & disable it constantly.
  3. It's only one option, so make less mess in the context menu (but it's still a compromise).

I really wish people would have commented on the core idea presented in the issue, BEFORE I decided to go to the trouble to code it and prepare a PR.
I took a lack of response in the issue comments as being tacit approval, or certainly no real objection on the approach.
It is a shame to have to wait for PR time for people to start offering opinions. :-(

Sorry about that. I'm overwhelmed by all the issues and I cannot keep an eyes on all the issue activities (especially when I'm coding). I'll be more aware of the issue activities.
As we mentioned before, if you are about to implement somethings more complex, it's always good to drop me a mail. So we can check it together.

@sasumner
Copy link
Contributor

These also seem to be somewhat of "settings within menus":

image

image


Do the settings I've proposed above belong in the Preferences section?
Or maybe they aren't necessary at all...

I think that tucking what I've proposed into the spot pointed out by @ArkadiuszMichalski might be a valid "exception" -- it feels better to me to be contextually close to the functions it affects -- much like the "save as *.*..." is better in the SaveAs dialog than located far away in the Preferences.


it's always good to drop me a mail

I suppose the mail could say "can you have a look at issue # xxxx comments?"
I thought an @ donho (from a frequent contributor) should have the same effect.

@donho
Copy link
Member

donho commented Apr 15, 2021

Do the settings I've proposed above belong in the Preferences section?

Not for me. They are actions to turn each visual effect ON or OFF.

Another point in your suggestion bothers me is these options for "Mark All".
I believe we can do better than 6 options (it's too many options for "Mark all").

Putting "mark all" feature options aside, this issue (bug) is about Style Token not applied consistently, so I prefer to focus on fixing the bug than reconfiguring matching token possibility.

I suppose the mail could say "can you have a look at issue # xxxx comments?"

Yes, please.

I thought an @ donho (from a frequent contributor) should have the same effect.

No, I get a notification for every single issue/pr and all the conversation:

image

I have to click on each message to find if I'm mentioned.

@donho donho self-assigned this Apr 15, 2021
@sasumner
Copy link
Contributor

Do the settings I've proposed above belong in the Preferences section?

Not for me. They are actions to turn each visual effect ON or OFF.

I think we have a misunderstanding.
I'm asking about the new settings proposed by the issue/PR, not Wrap etc.


Another point in your suggestion bothers me is these options for "Mark All".
I believe we can do better than 6 options (it's too many options for "Mark all").

The 6 options allow ONE visit to the menu to set what you want.
But...the PR did streamline this down to 4 choices (but if you want both case and word options to change, you have to visit the place to set them TWICE).


this issue (bug) is about Style Token not applied consistently, so I prefer to focus on fixing the bug than reconfiguring matching token possibility.

Hmmm.
Why not fix the bug from this issue and also 5148 by also advancing functionality?


I thought an @ donho (from a frequent contributor) should have the same effect.

No, I get a notification for every single issue/pr and all the conversation:

Ouch.
Well, now I know.
But a suggestion is to set up some filters.

@sasumner
Copy link
Contributor

donho self-assigned this 14 hours ago

Does this mean you are taking this issue over from me?
If so, please unassign me.

Also note that this is a bit of an involved issue that I have done a complete investigation on already.
So, proceed carefully is some good advice. :-)
The last time such a takeover occurred it resulted in a new bug making it into released code.

@Ekopalypse
Copy link
Contributor

If there was a way to always set "style token" to "whole word" matching, that would be nice for my workflow. :-)

@donho
Copy link
Member

donho commented Apr 16, 2021

Well, now I know.
But a suggestion is to set up some filters.

Yes, I just set the filter "@donho", it seems work fine - I won't have any excuse from now on :)

Does this mean you are taking this issue over from me?
If so, please unassign me.

If I wanted to take over this issue, I would ask you before, then unassigned you. But it's not my intention.
What I want is to have more discussion and have a consensus about this issue to prevent from that your work in vain.
By assigning me make me easy to find the thread for the discussion.

I'm asking about the new settings proposed by the issue/PR, not Wrap etc.

Yes & no. Yes for the principle: such settings should always be in Preferences dialog.
By no I mean I'm not sure it's a good idea to providing all the options for this feature.

This issue is certainly a bug because the behaviour is not consistent. The fix for me is to define a consistent way. What @Ekopalypse has suggested (always set "style token" to "whole word" matching) could be the final solution.

@sasumner
Copy link
Contributor

This issue is certainly a bug because the behaviour is not consistent. The fix for me is to define a consistent way.

That is a good thought, except, from the data we have, there are 2 opposing opinions on a consistent way:

OP says:

I suggested matching within words.

Eko says:

always set to ... "whole word" matching

@dinkumoil
Copy link
Contributor

I think this is a twofold problem:

  1. The way how styling works depends on settings of the F&R dialog. This is not obvious for most users. Solution: It should be optional and off by default. The current behaviour should be considered as a design flaw.

  2. The dependence to settings of the F&R dialog takes only place after this dialog has been opened at least once. Before its first opening some default values for the "Match case" and "Match whole word only" settings are used which are likely hardcoded into Npp's source. Solution: The dependence to settings of the F&R dialog should take place always. It should not matter whether the F&R dialog has been opened before or not. The current behaviour should considered as a bug.

We have a similar feature called "Smart Highlighting": Double-clicking a word highlights other instances of the same word (or words that contain the clicked word, depending on option "Match whole word only", see below) in the whole text. These are the options to configure this feature:

grafik

The three checkboxes in the red square are coupled together:

  • It is possible to check "Match case" and "Match whole word only" at the same time but if one of them is toggled from unchecked to checked the "Use Find dialog settings" checkbox becomes unchecked.
  • If you check the "Use Find dialog settings" checkbox the other two checkboxes become unchecked.

Notepad++ currently lacks a similar logic consisting of the same three settings for the styling feature. Maybe the preferences dialog's "Highlighting" group could be renamed to "Highlighting and Styling" to add a new groupbox entitled "Styling" which would contain the settings "Match case", "Match whole word only" and "Use Find dialog settings". Maybe the setting "Style in other view" could be added as well. The default values of these options could be the same as the ones for the "Smart Highlighting" feature.

@donho
Copy link
Member

donho commented Apr 23, 2021

@sasumner The consist way is on the settings of Style token (similar as settings of Smart Highlighting, but it should have only Match case, Match whole word only but without Use Find dialog settings). Style toke settings should be in Highlighting section.

@donho
Copy link
Member

donho commented Apr 23, 2021

@dinkumoil

  • It is possible to check "Match case" and "Match whole word only" at the same time but if one of them is toggled from unchecked to checked the "Use Find dialog settings" checkbox becomes unchecked.
  • If you check the "Use Find dialog settings" checkbox the other two checkboxes become unchecked.

The current behaviour IS already the one described above.

"Highlighting and Styling" to add a new groupbox entitled "Styling" which would contain the settings "Match case", "Match whole word only" and "Use Find dialog settings". Maybe the setting "Style in other view" could be added as well.

There's not enough room for a detailed description, and the translation will be even longer than English text, so I would say "Highlighting" is a good compromise.

But you're right for 3 settings Match case, Match whole word only & Use Find dialog settings: they should be regroup separately from the 2 main options.

@dinkumoil
Copy link
Contributor

@donho

The current behaviour IS already the one described above.

I'm not sure if you understood me. Thus I created a mockup of my imaginary GUI:

grafik

@donho
Copy link
Member

donho commented Apr 24, 2021

@dinkumoil
Yes, I did understand what you mean.

That's what I would do:

image

@sasumner
Copy link
Contributor

sasumner commented Apr 24, 2021

First, "Styling" isn't a known thing to users, really.
Well, ok, maybe via "Style token" on the context menu, but the main commands (on the main menu) relating to the feature don't really refer to it as "styling".
Perhaps this naming mismatch should be "corrected"?
Otherwise, I think it needs to have nomenclature tying it to "Mark All".

Second, these related commands are on the Search menu, so why wouldn't the Styling group box appear on the Searching subpage of the Preferences ?

@donho
Copy link
Member

donho commented Apr 24, 2021

@sasumner
It rather looks like:

image

Here's the code:

IDD_PREFERENCE_SUB_HIGHLIGHTING DIALOGEX 0, 0, 455, 185
STYLE DS_SETFONT | DS_FIXEDSYS | DS_CONTROL | WS_CHILD
FONT 8, "MS Shell Dlg", 0, 0, 0x1
BEGIN
    GROUPBOX        "Smart Highlighting",IDC_SMARTHILITING_STATIC,226,29,172,108,BS_CENTER
    CONTROL         "Enable",IDC_CHECK_ENABLSMARTHILITE,"Button",BS_AUTOCHECKBOX | BS_MULTILINE | WS_TABSTOP,234,39,142,10
    CONTROL         "Match case",IDC_CHECK_SMARTHILITECASESENSITIVE,"Button",BS_AUTOCHECKBOX | BS_MULTILINE | WS_TABSTOP,241,86,142,10
    CONTROL         "Match whole word only",IDC_CHECK_SMARTHILITEWHOLEWORDONLY,
                    "Button",BS_AUTOCHECKBOX | BS_MULTILINE | WS_TABSTOP,241,99,142,10
    CONTROL         "Use Find dialog settings",IDC_CHECK_SMARTHILITEUSEFINDSETTINGS,
                    "Button",BS_AUTOCHECKBOX | BS_MULTILINE | WS_TABSTOP,241,113,142,10
    CONTROL         "Highlight another view",IDC_CHECK_SMARTHILITEANOTHERRVIEW,
                    "Button",BS_AUTOCHECKBOX | BS_MULTILINE | WS_TABSTOP,234,55,142,10
    GROUPBOX        "Highlight Matching Tags",IDC_TAGMATCHEDHILITE_STATIC,62,82,155,55,BS_CENTER
    CONTROL         "Enable",IDC_CHECK_ENABLTAGSMATCHHILITE,"Button",BS_AUTOCHECKBOX | BS_MULTILINE | WS_TABSTOP,71,92,140,10
    CONTROL         "Highlight tag attributes",IDC_CHECK_ENABLTAGATTRHILITE,
                    "Button",BS_AUTOCHECKBOX | BS_MULTILINE | WS_TABSTOP,71,106,140,10
    CONTROL         "Highlight comment/php/asp zone",IDC_CHECK_HIGHLITENONEHTMLZONE,
                    "Button",BS_AUTOCHECKBOX | BS_MULTILINE | WS_TABSTOP,71,120,140,10
    GROUPBOX        "Mark All",IDC_SMARTHILITING_STATIC2,62,29,155,45,BS_CENTER
    CONTROL         "Match case",IDC_CHECK_SMARTHILITECASESENSITIVE2,"Button",BS_AUTOCHECKBOX | BS_MULTILINE | WS_TABSTOP,70,42,142,10
    CONTROL         "Match whole word only",IDC_CHECK_SMARTHILITEWHOLEWORDONLY2,
                    "Button",BS_AUTOCHECKBOX | BS_MULTILINE | WS_TABSTOP,70,55,142,10
    GROUPBOX        "Matching",IDC_TAGMATCHEDHILITE_STATIC2,233,74,155,55,BS_CENTER
END

Please replace all the symbol IDs generated by VS.

@donho
Copy link
Member

donho commented Apr 24, 2021

And please keep "Highlighting" section name unchanged.

@sasumner
Copy link
Contributor

7.9.5 Highlighting subpage looks like this:

image

So I understand this addition in the context of this current issue:

image

But WTF is this??:

image

@ArkadiuszMichalski
Copy link
Contributor

ArkadiuszMichalski commented Apr 24, 2021

@sasumner Adding label for this 3 option? Note that these are the same options as before, still placed in Smart Highlighting.
image

@sasumner
Copy link
Contributor

Oh.
Wow.
Those group box container lines do not show up very well at all on my browser/monitor, with that grey background!
I can see them now but my original thought was that there were 4 independent group boxes here.

@donho
Copy link
Member

donho commented Apr 24, 2021

As @dinkumoil said, "Use Find dialog settings" option is not obvious for the most of users. Putting them together and adding "Match" label may make this option more explicit.

@sasumner
Copy link
Contributor

OK, I will finish this one up soon as I am eager to get it off my plate.
The final design is one that is not reasonable for me (as a user), but I still have my scripts which do make "styling" workable for my use cases.
I'm not convinced that this is a widely used feature, anyway, due to the fact that any applied styles are lost upon N++ restart.

@dinkumoil
Copy link
Contributor

@donho

It is still unclear to me what will happen with the coupling of the styling/marking feature to F&R dialog settings. Since you refused to introduce an option to toggle that coupling, I hope it will be removed entirely. That's the reason why I wrote:

The way how styling works depends on settings of the F&R dialog. This is not obvious for most users.

Having an option to toggle the coupling would allow users who like this coupling to continue working as they are used to. The other ones would be able to disable the coupling. Without having this option IMO it would be the best to remove the coupling in the source code.

@sasumner
Copy link
Contributor

Without having this option IMO it would be the best to remove the coupling in the source code.

With current direction, this will be true.

@donho
Copy link
Member

donho commented Apr 24, 2021

@dinkumoil

Since you refused to introduce an option to toggle that coupling,

Sorry, what are you talking about?

@dinkumoil
Copy link
Contributor

@donho

Your mock-up of the future UI:

image

There is no checkbox in "Mark All" groupbox to toggle coupling of styling/marking feature to F&R dialog settings.

@donho
Copy link
Member

donho commented Apr 24, 2021

What do you mean by "an option to toggle that coupling"?

@dinkumoil
Copy link
Contributor

dinkumoil commented Apr 24, 2021

With "coupling" I mean that currently the way how the styling/marking feature works depends on settings in the F&R dialog.

Marking takes into account the state of "Match whole word only" and "Match case" options from F&R dialog. But this is only true if F&R dialog has been opened at least once. Before its first opening it seems the marking feature uses some default values for "Match whole word only" and "Match case" options which presumably are hardcoded into N++ source code.

@donho
Copy link
Member

donho commented Apr 24, 2021

Marking takes into account the state of "Match whole word only" and "Match case" options from F&R dialog. But this is only true if F&R dialog has been opened at least once. Before its first opening it seems the marking feature uses some default values for "Match whole word only" and "Match case" options which presumably are hardcoded into N++ source code.

@dinkumoil My intention is dissociate the settings of "Mark All" from "Smart Highlighting".
That's why the new UI - by adding the 2 options for "Mark All", it means the feature in question uses only its "Match whole word only" and "Match case" but doesn't use anymore the settings of "Smart Highlighting", so no more "Use Find dialog settings" for "Mark All".
@sasumner I hope we are on the same page.

@sasumner
Copy link
Contributor

That's why the new UI - by adding the 2 options for "Mark All", it means the feature in question uses only its "Match whole word only" and "Match case" but doesn't use anymore the settings of "Smart Highlighting", so no more "Use Find dialog settings" for "Mark All".

This confuses me somewhat because the "Mark All" feature never used the "Smart Highlighting" settings.
The "Mark All" feature currently uses ONLY the settings from the "Find" dialog, and there are bugs relating to usage of "Mark All" before the "Find" dialog has been opened/used the first time.
So, while we can create NEW settings for these, I just wanted to make it clear that the existing feature does not use "Smart Highlighting" settings.
Hopefully this makes things clearer.
At this point, I will proceed with the most-recent @donho direction...

@dinkumoil
Copy link
Contributor

@sasumner

"Mark All" feature never used the "Smart Highlighting" settings. The "Mark All" feature currently uses ONLY the settings from the "Find" dialog

That's what I wanted to write too.

@donho
Copy link
Member

donho commented Apr 24, 2021

"Mark All" feature never used the "Smart Highlighting" settings. The "Mark All" feature currently uses ONLY the settings from the "Find" dialog

Oups, my bad. I didn't check the code and I thought that "Mark All" feature is using the "Smart Highlighting" settings, including its "Use Find dialog settings" option.

By separating the UI, I hope it'll be clear for users.

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