-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Conversation
Hmm...okay...signed/unsigned mismatch...will fix. |
PowerEditor/src/NppBigSwitch.cpp
Outdated
{ | ||
tabIndex = pDt->getCurrentTabIndex(); | ||
} | ||
if ((tabIndex >= 0) && (tabIndex < pDt->nbItem())) |
There was a problem hiding this comment.
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 firststatic_cast
in l. 2958. Comparing unsigned values with-1
works exactly as you would expect; the literal-1
will be implicitly converted to an unsignedDWORD
.
Never mind that last part; it would cause a narrowing conversion in 64-bit builds.
Just cast nbItem()
's return value to an int
.
There was a problem hiding this 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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
That's rather complicated from an end-user perspective. Trying to encode properly, and knowing whether that means
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 |
Indeed,
It makes sense to me. @alankilborn |
@donho said:
I think Peter, as usual, makes a great deal of sense. |
The only "trouble" remaining before I submit revisions is this:
(BTW, I'm not sure why I originally had doubled up the 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". 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. :-) |
@alankilborn
I would use tab color ID instead. 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? |
Sounds good. |
I would use |
There was a problem hiding this 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.
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.