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

Added feature: we can set colour for individual tabs #12098

Closed
wants to merge 8 commits into from

Conversation

spaxio
Copy link
Contributor

@spaxio spaxio commented Sep 4, 2022

Fixes: #2271

This pull request implements a feature where we can select a colour for an individual tab.

We can choose up to 5 hues (red/green/blue/orange/purple) in View/Tab/Color menu.
Colours are saved in session.xml as integer, where -1 (or no value at all) means no colouring.

I've selected 5 because in my opinion, anything more would get more confusing with lots of tabs.

This is how it looks like:

  1. https://i.imgur.com/lutHHgB.jpg (lightmode with darkened inactive tabs)
  2. https://i.imgur.com/ahtDt7x.jpg (lightmode)
  3. https://i.imgur.com/Rlgt94T.png (darkmode)

It works in vertical mode too.
The colour is not retained when we try to pull a tab into a separate window.

Things worth discussing

  • Should we allow user to select their own hues in the settings (right now they are fixed).
  • Do we want to add possibility to select colour from tab context menu?
  • When the tab is active then colouring is skipped, but maybe we could also colourize that top thin bar (which is orange by default)?

@ArkadiuszMichalski
Copy link
Contributor

ArkadiuszMichalski commented Sep 4, 2022

Answer for asked question:

  1. Yes, a few fixed values may not meet everyone's expectations.
  2. Yes, it will be more convenient to set this in the tab context menu (at least for me).
  3. I have no opinion, but if necessary in the theme settings in Global Style> Active tab focused we can add a switch [ ] Overwrite with individual tab color.

I'm also wondering if there should be an overwrite option for Inactive tab by this individual tab color (because these individual tab colors are poorly visible together with the default inactive gray).

@alankilborn
Copy link
Contributor

Should we allow user to select their own hues in the settings

Yes, like styles 1 - 5 are available there; it would be a disappointment to users to not be able to customize

Do we want to add possibility to select colour from tab context menu?

Yes, with color samples, like styling:

image

Also, in a submenu (again like styling) would be appreciated.

@Yaron10
Copy link

Yaron10 commented Sep 5, 2022

@spaxio,

Thank you for your work. 👍

Would it be possible to apply the selected color to the active tab?
Isn't it confusing that the user does not see any effect to the color selection until after switching tabs?

@alankilborn
Copy link
Contributor

Would it be possible to apply the selected color to the active tab?
Isn't it confusing that the user does not see any effect to the color selection until after switching tabs?

Wouldn't it be better served to only change the color of the narrow strip of colour in the top of the active tab to the colour user just selected?
I really don't want the active tab's entire colour to possibly match other tabs -- I really want it to stand out..it's the active tab, after all.

@Yaron10
Copy link

Yaron10 commented Sep 5, 2022

@alankilborn,

We agree that when the user is selecting a color, they should see some effect before switching tabs.

And I certainly see your point.
But wouldn't it still be confusing if that effect is initially on the upper strip, and then (on switching tabs) on the tab's background color?

Wouldn't the upper strip suffice to make the active tab stand out?

As you and the others understand. :)

@alankilborn
Copy link
Contributor

But wouldn't it still be confusing if that effect is initially on the upper strip, and then (on switching tabs) on the tab's background color?

Well, I haven't actually seen it, but to me that doesn't sound confusing.

Wouldn't the upper strip suffice to make the active tab stand out?

Not sure at all what this means, in the context of the recent conversation.

@Yaron10
Copy link

Yaron10 commented Sep 5, 2022

Wouldn't the upper strip suffice to make the active tab stand out?

Not sure at all what this means, in the context of the recent conversation.

In response to your previous comment:

I really don't want the active tab's entire colour to possibly match other tabs -- I really want it to stand out..it's the active tab, after all.

@alankilborn
Copy link
Contributor

I suppose if the upper colour strip is really garish it would help it stand out. But I wouldn't necessarily want the tab coloring that garish on other tabs of the same color.

@donho donho self-assigned this Sep 6, 2022
@spaxio
Copy link
Contributor Author

spaxio commented Sep 6, 2022

Ok,

So I've added colourization of active bar highlight and in dark mode (default grey) it look awesome. But I'm not so sure about light ones (default theme, and with darkened inactive tabs).

I'm also not sure about adding "Set colour" item to the document context menu since those should be related only to document itself. I would much more prefer to add it to the Tab context menu but the way it's implement right now I'm not able to add sub-menus there. For now I think it's best to leave it in View/Tab/Colour.

@alankilborn
Copy link
Contributor

I'm also not sure about adding "Set colour" item to the document context menu

Correct. Don't do it.

I would much more prefer to add it to the Tab context menu

That could be a later change.
Although not having it will be a sad omission.
And it will be the first thing that users that want to colour tabs will request.

@alankilborn
Copy link
Contributor

I tried it out in dark mode (black) and the effect is OK:

image

For the other dark modes (red, green, blue, etc), sometimes the effect seems better than others.

In non-dark mode it is less striking (as noted by @spaxio):

image

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. Move the 3rd level menu items to the 2nd level, and rename the label for the future customization.
  2. Add these menu items also on the context menu of tab.
  3. Add Color square to the menu items (as Style token).
  4. If possible, the focused (the current selected) tab has the assigned colour on top bar, if it is activated.

The customization of colour is out of scope and will be in an another PR

PowerEditor/src/Notepad_plus.rc Outdated Show resolved Hide resolved
@donho
Copy link
Member

donho commented Sep 8, 2022

BTW, I notice this PR is not from a new branch:

Create a new branch for each PR. Make sure your branch name wasn't used before - you can add date (for example patch3_20200528) to ensure its uniqueness.

https://github.com/notepad-plus-plus/notepad-plus-plus/blob/master/CONTRIBUTING.md#guidelines-for-pull-requests

It's OK for the first time because there will no impact to the workflow of merging this PR. Please follow the suggestion for your next PR.

@spaxio
Copy link
Contributor Author

spaxio commented Sep 9, 2022

All points have been addressed. 🎉
I also adjusted colors for default themes (red, cyan, olive etc.). It looks better than before.

@spaxio spaxio requested a review from donho September 9, 2022 17:21
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 fix the localization bug with the suggested solution.

PowerEditor/src/NppNotification.cpp Show resolved Hide resolved
PowerEditor/src/localization.cpp Outdated Show resolved Hide resolved
@ArkadiuszMichalski
Copy link
Contributor

Maybe for another PR, but it would be nice to have these colors also for items in the Document List panel.

@alankilborn
Copy link
Contributor

With the addition of six items to the tab context menu, the menu seems to be getting to an almost obnoxious vertical size:

image

Shouldn't the new items be at the bottom? Users may complain that to "move to another instance" they now have to move the mouse a huge amount to reach it. Of course, users always complain... :-)

I'm not sure if there is a "typical" size monitor that Notepad++ (maximized) is targeting. In days of old, it might have been 1024 x 768... Not sure about now. But the point is, perhaps the tab context menu is getting dangerously close to appearing from top-screen-edge to bottom-screen-edge, and possibly even with the dreaded ▲ at the top and ▼ at the bottom.

Is it time to bite the bullet and do some submenus here? There are obvious groupings (these "colours", the "close all..."s, the "opens", the "clipboards").

@donho Would this be considered? Should I make an issue?

@alankilborn
Copy link
Contributor

Maybe for another PR, but it would be nice to have these colors also for items in the Document List panel.

Sounds like a new issue. :-)

@spaxio
Copy link
Contributor Author

spaxio commented Sep 10, 2022

With the addition of six items to the tab context menu, the menu seems to be getting to an almost obnoxious vertical size

I agree, I would remove those. Features discovery will suffer but we can add those when submenu gets implemented. Or make it dependent on a flag in settings (in the future)

@donho
Copy link
Member

donho commented Sep 10, 2022

I agree, I would remove those. Features discovery will suffer but we can add those when submenu gets implemented. Or make it dependent on a flag in settings (in the future)

It's shame to remove this feature from the tab context menu, that will make tab coloring meaningless.
OTOH sub-menu would be a good solution.

How about the following new tab context menu?

----------------------
Close                  
Close More           🢒
Save
Save As...
Open into            🢒
Rename
Reload
Move to Recycle Bin
Print
----------------------
Read Only
Clear Read Only Flag
Clipboard            🢒
Document to          🢒
----------------------
Apply Color          🢒
----------------------

Close More contains :

  • Close ALL BUT This
  • Close ALL to the Left
  • Close ALL to the Right
  • Close ALL Unchanged

Open contains:

  • Open Containing Folder in
  • Open Containing Folder in
  • Open Containing Folder as

  • Open in Default Viewer

Clipboard contains:

  • Full File Path to Clipboard
  • Filename to Clipboard
  • Current Dir. to Clipboard

Document to contains:

  • Move to Other View
  • Clone to Other View
  • Move to New Instance
  • Open in New Instance

Apply Color contains:

  • Apply Color 1
  • Apply Color 2
  • Apply Color 3
  • Apply Color 4
  • Apply Color 5
  • Remove Color

If you guys think the idea of another simpler context menu is OK, then I will implement it in another PR.

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 fix the typo and the variable naming.

PowerEditor/src/ScintillaComponent/DocTabView.h Outdated Show resolved Hide resolved
PowerEditor/src/NppDarkMode.cpp Outdated Show resolved Hide resolved
static constexpr IndividualTabColours individualTabHuesFor_DarkCyna { { HLSColour{0, 60, 60}, HLSColour{70, 60, 60}, HLSColour{144, 70, 60}, HLSColour{13, 60, 60}, HLSColour{195, 60, 60} } };
static constexpr IndividualTabColours individualTabHuesFor_DarkOlive { { HLSColour{0, 60, 60}, HLSColour{70, 60, 60}, HLSColour{144, 60, 60}, HLSColour{13, 60, 60}, HLSColour{195, 60, 60} } };

static const IndividualTabColours individualTabHues { { HLSColour{0, 210, 150}, HLSColour{70, 210, 150}, HLSColour{144, 210, 150}, HLSColour{13, 210, 150}, HLSColour{195, 210, 150} } };
Copy link
Member

Choose a reason for hiding this comment

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

I see that individualTabHues (light theme) and all other 7 IndividualTabColours individualTabHuesFor_DarkXXX arrays contains different 5 colour values.

It will be a major problem in the future if we want to make these 5 colours customizable.
Is it possible to have a function in which we pass a RGB value, and choice (light, dark, darkRed, darkBlue...) then the function in question return the RGB we need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that this is just implementation detail and what we are exposing is RGB COLORREF getIndividualTabColour(...)
So if in the future we want to add colour customization we only have to add a case for if(g_colorToneChoice == customizedTone).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to have a function in which we pass a RGB value, and choice (light, dark, darkRed, darkBlue...) then the function in question return the RGB we need?

And yes we can transform RGB into just a Hue and modify it according to theming rules back to RGB.

Copy link
Member

Choose a reason for hiding this comment

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

@spaxio

and modify it according to theming rules back to RGB.

When you say "modify it" you mean add it manually to other 7 vector right?
But if the customized color is completely dynamic added by user as we does in Style Configurator?
image

@ozone10
Copy link
Contributor

ozone10 commented Sep 11, 2022

Not really fan of "... to Clipboard", for submenus I would use

  • Copy Full File Path
  • Copy Filename
  • Copy Current Dir.

It's necessary to rename these items in the main menu. I will see what I can do about it.

Ok.
The other day I need to copy full path name and could not find it immediately, as I am used to term "Copy".

I would use
Close More -> Close Multiple Tabs

@alankilborn is right, Close More reuses the main menu term, with the same sub-menu items. So it should be remained.

I see, I did not know about main menu, I rarely use it. I've used term "Close Multiple Tabs" to make it more familiar to users, as internet browsers are using it.

@spaxio spaxio requested a review from donho September 11, 2022 11:16
@alankilborn
Copy link
Contributor

What value are the SEPARATORS in @donho's #12098 (comment) ?

image

They just make a user have to move the mouse further to get where they are going.

OK, maybe the one after Print is reasonable, but why have the one between "Move Document 🢒" and "Apply Color 🢒"?

Really there SHOULD be a separator between "Clear Readonly Flag" and "Copy to Clipboard 🢒" but even better than that would be putting "Read Only" and "Clear RO Flag" into a "Readonly 🢒" submenu.

"Open into 🢒" seems a bit obscure for opening Explorer, is there anything wrong with "Open Containing Folder 🢒" which is really obvious?

Summary of all my suggestions in this post:

Close
Close More             ▶
Save
Save As...
Open Containing Folder ▶
Rename
Reload
Move to Recycle Bin
Print
------------------------
Read Only              ▶  -----+---- Set Read-Only
                               |---- Clear Read-Only Flag
Copy to Clipboard      ▶
Move Document          ▶
Apply Color to Tab     ▶

@donho donho added feature Feature requests and Feature commits accepted labels Sep 11, 2022
@donho donho closed this in 42d863d Sep 11, 2022
Mateos81 pushed a commit to Mateos81/notepad-plus-plus that referenced this pull request Sep 11, 2022
@donho
Copy link
Member

donho commented Sep 11, 2022

@alankilborn

What value are the SEPARATORS

The 1st part is about file operation which corresponds to the main menu "File" (Close, Save, Open, Delete, Print...).
The 2nd part is the OTHER operations and the 3rd part is the new added feature. But well, indeed, we can merge the 2nd & the 3rd parts in one section.
Note that there are only 2 Read-Only related commands, and Read-Only could be frequently used by users, so I prefer to keep both on the 1st level.

@alankilborn
Copy link
Contributor

Read-Only could be frequently used by users

I'm very dubious of that, but as you like.
Thank you for revising an overlong tab context menu as part of the addition of colored tabs.

@donho
Copy link
Member

donho commented Sep 11, 2022

@ArkadiuszMichalski

Maybe for another PR, but it would be nice to have these colors also for items in the Document List panel.

It'll be harder to do in my opinion (I have no idea how to implement it anyway).
You can always create a PR BTW.

@donho
Copy link
Member

donho commented Sep 12, 2022

Hmm... Finally Close Multiple Tabs makes more sense than Close More .
I will also modify in the main menu.

@alankilborn
Copy link
Contributor

@donho said:

Finally Close Multiple Tabs makes more sense than Close More . I will also modify in the main menu

If you are making these sorts of changes, consider removing the word "All" from each of the "Close Multiple Tabs" items.

It is awkward because "all" suggests, well, everything. It is a strong word in a native English speaker's mind.

Better would be:

  • Close Other Tabs (instead of Close All BUT This from right-click context menu, or Close All but Active Document from File menu -- why different text for the same thing in 2 places?)
  • Close Tabs to the Left
  • Close Tabs to the Right
  • Close Saved Tabs (instead of Close All Unchanged)

The use of "unchanged" introduces another term to Notepad++ users; why not just use "saved" as "save" is extremely solid in a user's mind?

This would bring commonality with current naming trends that browser's use and users would be more familiar with; example from Chrome:

image

Also, maybe this is another opportunity to promote #12072 as a really good idea. :-)

@donho
Copy link
Member

donho commented Sep 13, 2022

It is awkward because "all" suggests, well, everything. It is a strong word in a native English speaker's mind.

English is not my native language, but I just checked MS Studio, it uses also "Close All But this".
For the other suggestion, let's see if there are other users want to change them.

@Yaron10
Copy link

Yaron10 commented Sep 13, 2022

The Close commands are used frequently, and I'd highly recommend not to place them in a sub-menu.
When Firefox did that - many users found it inconvenient.

@alankilborn
Copy link
Contributor

The Close commands are used frequently, and I'd highly recommend not to place them in a sub-menu.

In direct opposition to that assertion, I find that Close itself is the only one of the group that is used frequently. Putting the others in a submenu is best with the benefit that it provides a level of safety (harder to accidentally invoke a "Close All..." with a mouse stutter).

I think with the possibility of some name tweaking, that the "close" options are perfectly situated.

@Yaron10
Copy link

Yaron10 commented Sep 13, 2022

@alankilborn,

You can disagree, and I respect your opinion.
If you want to comment and expect a response, please don't be rude!

@alankilborn
Copy link
Contributor

please don't be rude!

Confused. Please tell me where in my response I was rude.

@Yaron10
Copy link

Yaron10 commented Sep 13, 2022

@alankilborn,

I respect and appreciate you. Let's move on. :)

@donho
Copy link
Member

donho commented Sep 14, 2022

@Yaron10

The Close commands are used frequently, and I'd highly recommend not to place them in a sub-menu.
When Firefox did that - many users found it inconvenient.

Just check the firefox's tab context menu - the sub-menus are still there.
But I do understand your point - I'll try to make it customizable completely.
Could you create an issue (FR) for that?

@ArkadiuszMichalski
Copy link
Contributor

@donho #12170

@Yaron10
Copy link

Yaron10 commented Sep 14, 2022

@donho,

Yes, Firefox did not revert that change.

But I do understand your point - I'll try to make it customizable completely.

👍
Thank you.

If you think it's worth the extra work and code. :)

@ArkadiuszMichalski,

👍
Thank you.

@alankilborn
Copy link
Contributor

I'll try to make it customizable completely.

Personally, I would love this; having tabContextMenu.xml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted feature Feature requests and Feature commits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Colored tabs
6 participants