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

Add plugin command NPPM_GETTABCOLORID #15142

Closed
wants to merge 3 commits into from

Conversation

alankilborn
Copy link
Contributor

Fix #15115

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
Copy link
Contributor Author

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

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

@rdipardo rdipardo May 15, 2024

Choose a reason for hiding this comment

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

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.

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 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)

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

// 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)
Copy link
Member

Choose a reason for hiding this comment

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

Same naming error as above.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

// 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)


#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.
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor

pryrt commented May 16, 2024

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.

@chcg chcg added plugin enhancement Proposed enhancements of existing features labels May 16, 2024
@donho
Copy link
Member

donho commented May 16, 2024

@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
Copy link
Contributor Author

@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 alankilborn changed the title Add plugin commands NPPM_GET_TABCOLORINDEX and NPPM_SET_TABCOLORINDEX Add plugin command NPPM_GETTABCOLORNUMBER May 16, 2024
@alankilborn
Copy link
Contributor Author

@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:

image

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
Copy link
Member

donho commented May 17, 2024

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

  1. tab color index has no meaning to me and to other dev I believe.
  2. it's confusing with "tab index".
  3. 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
Copy link
Contributor Author

What do you think?

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

@ozone10
Copy link
Contributor

ozone10 commented May 17, 2024

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

@alankilborn
Copy link
Contributor Author

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

@alankilborn alankilborn changed the title Add plugin command NPPM_GETTABCOLORNUMBER Add plugin command NPPM_GETTABCOLORID May 18, 2024
@donho donho self-assigned this May 19, 2024
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 consider the following suggestions.

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
@MarkusBodensee
Copy link
Contributor

MarkusBodensee commented May 22, 2024

@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 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted enhancement Proposed enhancements of existing features plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Allow plugins to get/set the existing color choice for a tab
7 participants