Add plugin command NPPM_GETTABCOLORID #15142

alankilborn · 2024-05-15T12:00:11Z

Note that the implementation I chose was to allow "setting" only for the active tab in the active view. "Getting", however, can be done on any tab in either view. Mainly this choice was to "keep it simple", but in reality it isn't that big of an "ask" to whatever calls the "set" to make the tab active first.

Other choices were arbitrary as well, e.g. using -1 to refer to the active tab (or view). I don't know if these choices will be liked.

alankilborn · 2024-05-15T15:07:30Z

Hmm...okay...signed/unsigned mismatch...will fix.

rdipardo · 2024-05-15T19:46:12Z

PowerEditor/src/NppBigSwitch.cpp

+			{
+				tabIndex = pDt->getCurrentTabIndex();
+			}
+			if ((tabIndex >= 0) && (tabIndex < pDt->nbItem()))


nbItem() returns a size_t.

~~[!TIP]~~
You could actually dispense with that first static_cast in l. 2958. Comparing unsigned values with -1 works exactly as you would expect; the literal -1 will be implicitly converted to an unsigned DWORD.

Never mind that last part; it would cause a narrowing conversion in 64-bit builds.

Just cast nbItem()'s return value to an int.

donho

Please consider the suggested API:

// int NPPM_GETTABCOLOR(int inView, int index2Activate)
// Get the tab color ID with given tab index and view.
// wParam[in]: inView - main view (0) or sub-view (1) or -1 (active view)
// lParam[in]: index2Activate - index (in the view indicated above). -1 for currently active tab
// Return tab color ID which contains the following values: 0 (yellow), 1 (green), 2 (blue), 3 (orange), 4 (pink) or -1 (no color)

donho · 2024-05-15T21:06:05Z

PowerEditor/src/MISC/PluginsManager/Notepad_plus_msgs.h

@@ -964,6 +964,19 @@ enum Platform { PF_UNKNOWN, PF_X86, PF_X64, PF_IA64, PF_ARM64 };
 	// if isAllocatedSuccessful is TRUE, and value of idBegin is 7
 	// then indicator ID 7 is preserved by Notepad++, and it is safe to be used by the plugin.

+	#define NPPM_NPPM_GET_TABCOLORINDEX (NPPMSG + 114)
+	// int NPPM_NPPM_GET_TABCOLORINDEX(int tabIndex, int inView)


NPPM_NPPM_GET_TABCOLORINDEX should be NPPM_GET_TABCOLORNUMBER IMO.
The reason: avoid the confusion between color index and tab index.

donho · 2024-05-15T21:06:32Z

PowerEditor/src/MISC/PluginsManager/Notepad_plus_msgs.h

+	// lParam[in]: inView, use 0 (main view) or 1 (sub view) or -1 (active view)
+	// Return -1 if the tab has no color, otherwise the current color index (0 - 4).
+
+	#define NPPM_NPPM_SET_TABCOLORINDEX (NPPMSG + 115)


Same naming error as above.

donho · 2024-05-15T21:12:16Z

PowerEditor/src/MISC/PluginsManager/Notepad_plus_msgs.h

@@ -964,6 +964,19 @@ enum Platform { PF_UNKNOWN, PF_X86, PF_X64, PF_IA64, PF_ARM64 };
 	// if isAllocatedSuccessful is TRUE, and value of idBegin is 7
 	// then indicator ID 7 is preserved by Notepad++, and it is safe to be used by the plugin.

+	#define NPPM_NPPM_GET_TABCOLORINDEX (NPPMSG + 114)
+	// int NPPM_NPPM_GET_TABCOLORINDEX(int tabIndex, int inView)
+	// Get the current tab color index of the specified tab and view.


// Get the current tab color index of the specified tab and view. should be // Get the tab color number with given tab index and view.

But I think it'll be nice to explain what tab color number is. So I suggest to add the following comment:

// Get the tab color number with given tab index and view. // tab color number contains the following values: // 0 (yellow) // 1 (green) // 2 (blue) // 3 (orange) // 4 (pink)

donho · 2024-05-15T21:42:30Z

PowerEditor/src/MISC/PluginsManager/Notepad_plus_msgs.h

+
+	#define NPPM_NPPM_SET_TABCOLORINDEX (NPPMSG + 115)
+	// int NPPM_NPPM_SET_TABCOLORINDEX(int colorIndex)
+	// Set a new tab color index for the active tab in the active view.


There are a concept problem of this set of API:
For GET and SET operation, we set or get ONLY for the current (ie. active) tab, or we set or get for the any tab by indicating tabIndex & view. It's a bad idea to mix both.

pryrt · 2024-05-16T14:31:33Z

The set of the API is not coherent. Please consider the suggested API:

	// lParam[in]: VIEW & INDEX or -1 (active view)
		// VIEW takes 2 highest bits and INDEX (0 based) takes the rest (30 bits)
		// Here's the values for the view:
		//  MAIN_VIEW 0
		//  SUB_VIEW  1

That's rather complicated from an end-user perspective. Trying to encode properly, and knowing whether that means

0xFFFFFFFF = active ("-1" refers to the whole 32bit integer)
0x80000000 = SUB_VIEW ("1" in the MSB)
0x00000000 = MAIN_VIEW
... or
0xFFFFFFFF (-1) = active
0x40000000 = SUB_VIEW ("1" in a "2 highest bit" perspective is "01", which then encodes to this value)
0x00000000 = MAIN_VIEW
... or
0xC0000000 = active (because those "2 highest bits" are -1)
0x80000000 = SUB_VIEW
0x00000000 = MAIN_VIEW
... or
0xC0000000 = active (because those "2 highest bits" are -1)
0x40000000 = SUB_VIEW
0x00000000 = MAIN_VIEW

All four of those are reasonable interpretations of what you wrote. I think that would cause a lot of headache for plugin/script developers.

Since the IDM_VIEW_TAB_COLOUR_* menu-command-ids already provide a way to SET the color from a menu command (and menu commands can be activated by their IDM through any plugin), we don't really need a SET version of the API message. There are plenty of other messages that only have one of the two directions, so it wouldn't be inconsistent with the API to only have a getter for this.

To me, it would make more sense to drop the SET_ version, and instead just have GET_TABCOLORNUMBER(tabIndex, inView) -- that makes the interface simpler, and all the same functionality is available. I would find that a lot easier to use than having to do a complicated encoding to be able to indicate "active" or "MAIN_VIEW | index" or "SUB_VIEW | index" into a single message argument.

donho · 2024-05-16T16:38:11Z

@pryrt

Since the IDM_VIEW_TAB_COLOUR_* menu-command-ids already provide a way to SET the color from a menu command (and menu commands can be activated by their IDM through any plugin), we don't really need a SET version of the API message.

Indeed, NPPM_MENUCOMMAND can be use to replace NPPM_SETTABCOLORNUMBER.

To me, it would make more sense to drop the SET_ version, and instead just have GET_TABCOLORNUMBER(tabIndex, inView) -- that makes the interface simpler, and all the same functionality is available. I would find that a lot easier to use than having to do a complicated encoding to be able to indicate "active" or "MAIN_VIEW | index" or "SUB_VIEW | index" into a single message argument.

It makes sense to me.

@alankilborn
What do you think?

alankilborn · 2024-05-16T19:33:06Z

@donho said:

alankilborn, What do you think?

I think Peter, as usual, makes a great deal of sense.
I will rework the PR in the spirit of his excellent guidelines.

alankilborn · 2024-05-17T12:06:05Z

@donho @pryrt

The only "trouble" remaining before I submit revisions is this:

NPPM_NPPM_GET_TABCOLORINDEX should be NPPM_GET_TABCOLORNUMBER

(BTW, I'm not sure why I originally had doubled up the NPPM part. :-( )

When one looks at the menu for coloring a tab, reference:

then if we are considering a color number, then I'd say the numbering is 1, 2, 3, 4, 5. However, if one thinks in term of a color index, then one might see it as 0, 1, 2, 3, 4.

So, with the current guideline, the nomenclature would be "number" but we'd be returning basically an "index".
I realize that it might be a "fine" point, but I just wanted to be sure that calling it a "number" and returning a zero-based value for the color is really what we want to do?

My preference would be to call it a "number", return 1, 2, 3, 4, 5 for the colors, and return 0 for a tab without color. But, I often don't get what I want here. :-)

donho · 2024-05-17T15:41:25Z

@alankilborn
Your viewpoint about "number" is not wrong. However, I would try to avoid index because

tab color index has no meaning to me and to other dev I believe.
it's confusing with "tab index".
tab colors in the code is not array, so it's not index

I would use tab color ID instead.
For the API name, let's keep simple, so we can just use "NPPM_GETTABCOLOR".

Here's the suggested API specs:

// int NPPM_GETTABCOLOR(int inView, int index2Activate)
// Get the tab color ID with given tab index and view.
// wParam[in]: inView - main view (0) or sub-view (1) or -1 (active view)
// lParam[in]: index2Activate - index (in the view indicated above). -1 for currently active tab
// Return tab color ID which contains the following values: 0 (yellow), 1 (green), 2 (blue), 3 (orange), 4 (pink) or -1 (no color)

What do you think?

alankilborn · 2024-05-17T16:06:07Z

What do you think?

Sounds good.
I will implement it that way and update this PR.

ozone10 · 2024-05-17T17:47:30Z

I would use NPPM_GETTABCOLORID. NPPM_GETTABCOLOR can be confusing. Someone could expect that it return color (yellow, ...) directly, and not its ID.

alankilborn · 2024-05-17T21:01:39Z

I think @donho would approve of @ozone10 's suggestion, so I'll go ahead with that direction.

donho

Please consider the following suggestions.

PowerEditor/src/NppBigSwitch.cpp

MarkusBodensee · 2024-05-22T07:21:32Z

@donho @alankilborn @pryrt @ozone10 @rdipardo @chcg
This PR is such a great example how collaboration works and how it results in a solid implementation. Nice to read through it, thank you. Wonderful job guys :-)

Add plugin commands NPPM_GET_TABCOLORINDEX and NPPM_SET_TABCOLORINDEX

efd29fd

rdipardo reviewed May 15, 2024

View reviewed changes

donho requested changes May 15, 2024

View reviewed changes

chcg added plugin enhancement Proposed enhancements of existing features labels May 16, 2024

alankilborn changed the title ~~Add plugin commands NPPM_GET_TABCOLORINDEX and NPPM_SET_TABCOLORINDEX~~ Add plugin command NPPM_GETTABCOLORNUMBER May 16, 2024

alankilborn changed the title ~~Add plugin command NPPM_GETTABCOLORNUMBER~~ Add plugin command NPPM_GETTABCOLORID May 18, 2024

Add plugin command NPPM_GETTABCOLORID

9c61b9c

donho self-assigned this May 19, 2024

donho requested changes May 19, 2024

View reviewed changes

PowerEditor/src/NppBigSwitch.cpp Outdated Show resolved Hide resolved

PowerEditor/src/NppBigSwitch.cpp Show resolved Hide resolved

PowerEditor/src/NppBigSwitch.cpp Outdated Show resolved Hide resolved

Add plugin command NPPM_GETTABCOLORID

f77f9db

donho approved these changes May 20, 2024

View reviewed changes

donho added the accepted label May 20, 2024

donho closed this in 9244cd0 May 20, 2024

This was referenced May 21, 2024

document plugin command NPPM_GETTABCOLORID notepad-plus-plus/npp-usermanual#671

Open

Support new N++ plugin command NPPM_GETTABCOLORID bruderstein/PythonScript#337

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plugin command NPPM_GETTABCOLORID #15142

Add plugin command NPPM_GETTABCOLORID #15142

alankilborn commented May 15, 2024

alankilborn commented May 15, 2024

rdipardo May 15, 2024 •

edited

donho left a comment •

edited

donho May 15, 2024

donho May 15, 2024

donho May 15, 2024

donho May 15, 2024

pryrt commented May 16, 2024

donho commented May 16, 2024

alankilborn commented May 16, 2024

alankilborn commented May 17, 2024

donho commented May 17, 2024 •

edited

alankilborn commented May 17, 2024

ozone10 commented May 17, 2024

alankilborn commented May 17, 2024

donho left a comment

MarkusBodensee commented May 22, 2024 •

edited

Add plugin command NPPM_GETTABCOLORID #15142

Add plugin command NPPM_GETTABCOLORID #15142

Conversation

alankilborn commented May 15, 2024

alankilborn commented May 15, 2024

rdipardo May 15, 2024 • edited

Choose a reason for hiding this comment

donho left a comment • edited

Choose a reason for hiding this comment

donho May 15, 2024

Choose a reason for hiding this comment

donho May 15, 2024

Choose a reason for hiding this comment

donho May 15, 2024

Choose a reason for hiding this comment

donho May 15, 2024

Choose a reason for hiding this comment

pryrt commented May 16, 2024

donho commented May 16, 2024

alankilborn commented May 16, 2024

alankilborn commented May 17, 2024

donho commented May 17, 2024 • edited

alankilborn commented May 17, 2024

ozone10 commented May 17, 2024

alankilborn commented May 17, 2024

donho left a comment

Choose a reason for hiding this comment

MarkusBodensee commented May 22, 2024 • edited

rdipardo May 15, 2024 •

edited

donho left a comment •

edited

donho commented May 17, 2024 •

edited

MarkusBodensee commented May 22, 2024 •

edited