Added "Sort By" function under Window Menu by deebash · Pull Request #11190 · notepad-plus-plus/notepad-plus-plus

deebash · 2022-02-12T11:13:09Z

Added sorting function under Window Menu for issue #10393

Yaron10 · 2022-02-12T12:35:43Z

@deebash,

Thank you for this PR.
I've tested it and it works like a charm. Great work. 👍

Allow me some comments:

Wouldn't it be nicer if the "Sort By" menu appeared after the list of open documents?
List of open documents.
Separator.
Sort By.
Windows...
Isn't it possible to adapt and use the existing Sort functions?
If this PR is accepted, you'd be asked to provide localization for the new items.

deebash · 2022-02-12T12:42:44Z

Thanks @Yaron10

Sure, I'll change that. I placed it on top so that if large number of documents are opened users needs to scroll to bottom all the way down to reach this menu item. If you prefer the way that you mentioned, I'll do the change.
Actually I used the existing sort function code, but removed few lines which is related to list view processing in Windows.... Dialog box.
Sure, I'll do that.

Yaron10 · 2022-02-12T12:49:07Z

@deebash,

I placed it on top so that if large number of documents are opened...

The number of open documents is limited to 10.
If @donho approves, I don't see why it shouldn't be changed to 20.

Thanks again.

mere-human · 2022-02-12T16:53:20Z


+		case IDM_WINDOW_SORT_FN_ASC :
+		{
+			WindowsDlg _windowsDlg;


Underscore at the beginning is usually used only for member variables.
https://github.com/notepad-plus-plus/notepad-plus-plus/blob/master/CONTRIBUTING.md#member-variables

mere-human · 2022-02-12T16:54:10Z

+{
+	size_t count = (_pTab != NULL) ? _pTab->nbItem() : 0;
+	auto currrentTabIndex = _pTab->getCurrentTabIndex();
+	NMWINDLG nmdlg;


You can also use uniform initialization to avoid uninitialized members. Like this: nmdlg = {};

mere-human · 2022-02-12T16:57:03Z

+	nmdlg.curSel = currrentTabIndex;
+	nmdlg.code = WDN_NOTIFY;
+	nmdlg.nItems = static_cast<UINT>(count);
+	nmdlg.Items = new UINT[count];


In order to avoid a potential memory leak, you can use a RAII approach. Example:

std::vector<UINT> items(count); nmdlg.Items = items.data();

deebash · 2022-02-13T00:42:44Z

I did the changes as mentioned by @mere-human. Not given commit yet.

@donho, If you accept this PR, I'll add localisation text and change the menu item position based upon your suggestion and I'll add single commit with both changes.

Yaron10 · 2022-02-14T22:53:41Z

@deebash,

List of open documents.
Separator.
Sort By.
Windows...

A few months ago I modified some code in WindowsDlg.cpp for my personal use.
Amongst other things, I've added a separator between the list of open documents and the Windows... command.

I did that by adding the following lines in WindowsDlg.cpp after the for ( ; id <= IDM_WINDOW_MRU_LIMIT; ++id) loop.

::RemoveMenu(hMenu, 0, MF_SEPARATOR);
::InsertMenu(hMenu, IDM_WINDOW_WINDOWS, MF_SEPARATOR | MF_BYCOMMAND, 0, NULL);

Could you please let me know if you have a better solution?

Thank you.

deebash · 2022-02-15T15:28:27Z

Could you please let me know if you have a better solution?

@Yaron10 If you want to add menu item at the bottom, then you can use -1 as position. It'll append to bottom of the menu

Yaron10 · 2022-02-15T20:35:07Z

@deebash,

I'll wait for your final PR. Thank you.

deebash · 2022-02-16T12:26:54Z

Hi @donho, waiting for your approval on the menu position.

donho

Please consider the change according the comments of @mere-human.

@mere-human

3 changes mentioned by @mere-human done in this commit.

deebash · 2022-02-17T07:05:33Z

@donho @mere-human changes has been done and commit added.

mere-human · 2022-02-17T09:11:25Z

Looks good. I haven't tested it locally yet.

donho · 2022-02-17T11:02:52Z

+			WindowsDlg windowsDlg;
+			windowsDlg.init(_pPublicInterface->getHinst(), _pPublicInterface->getHSelf(), _pDocTab);
+			windowsDlg.sortFileSizeDSC();
+            windowsDlg.doSort();			


Indent problem?

The usual mixed tabs/spaces

Indent error fixed in this commit

donho

code refactoring.
solve the indent problem (in comment).

donho · 2022-02-17T11:30:59Z

+	nmdlg.code = WDN_NOTIFY;	
+	SendMessage(_hParent, WDN_NOTIFY, 0, LPARAM(&nmdlg));
+}
+


Refactoring: I would add a new method here with the following code in WindowsDlg.cpp:

void WindowsDlg::sort(int columnID, bool reverseSort) { refreshMap(); _currentColumn = columnID; _reverseSort = reverseSort; stable_sort(_idxMap.begin(), _idxMap.end(), BufferEquivalent(_pTab, _currentColumn, _reverseSort)); }

Then use the sort() method on the following methods, in WindowsDlg.h:

void WindowsDlg::sortFileNameASC() { sort(0, false); } void WindowsDlg::sortFileNameDSC() { sort(0, true); } void WindowsDlg::sortFilePathASC() { sort(1, false); } ...

Note that the coding conversion (braces position) should be respected:
in cpp:

void aClass::method1() { // codes here }

in h:

class aClass { public: void method1(); int method2() { return _x; // only one line code can be placed in *.h } private: int _x; }

donho · 2022-02-17T11:32:58Z

-
+	void doSortToTabs();
+	void doSort();	
+	void sortFileNameASC();


Put the one line definition code here, with the new method sort().

donho · 2022-02-17T11:35:19Z

+			WindowsDlg windowsDlg;
+			windowsDlg.init(_pPublicInterface->getHinst(), _pPublicInterface->getHSelf(), _pDocTab);
+			windowsDlg.sortFileSizeASC();
+            windowsDlg.doSort();			


Indent problem

donho · 2022-02-17T11:35:31Z

+			WindowsDlg windowsDlg;
+			windowsDlg.init(_pPublicInterface->getHinst(), _pPublicInterface->getHSelf(), _pDocTab);
+			windowsDlg.sortFileTypeDSC();
+            windowsDlg.doSort();			


Indent problem

donho · 2022-02-17T11:35:37Z

+			WindowsDlg windowsDlg;
+			windowsDlg.init(_pPublicInterface->getHinst(), _pPublicInterface->getHSelf(), _pDocTab);
+			windowsDlg.sortFileTypeASC();
+            windowsDlg.doSort();			


Indent problem

donho · 2022-02-17T11:35:45Z

+			WindowsDlg windowsDlg;
+			windowsDlg.init(_pPublicInterface->getHinst(), _pPublicInterface->getHSelf(), _pDocTab);
+			windowsDlg.sortFilePathDSC();
+            windowsDlg.doSort();			


Indent problem

donho · 2022-02-17T11:35:51Z

+			WindowsDlg windowsDlg;
+			windowsDlg.init(_pPublicInterface->getHinst(), _pPublicInterface->getHSelf(), _pDocTab);
+			windowsDlg.sortFileNameASC();
+            windowsDlg.doSort();			


Indent problem

donho · 2022-02-17T11:35:59Z

+			WindowsDlg windowsDlg;
+			windowsDlg.init(_pPublicInterface->getHinst(), _pPublicInterface->getHSelf(), _pDocTab);
+			windowsDlg.sortFileNameDSC();
+            windowsDlg.doSort();			


Indent problem

donho · 2022-02-17T11:36:07Z

+			WindowsDlg windowsDlg;
+			windowsDlg.init(_pPublicInterface->getHinst(), _pPublicInterface->getHSelf(), _pDocTab);
+			windowsDlg.sortFilePathASC();
+            windowsDlg.doSort();			


Indent problem

donho · 2022-02-17T11:37:51Z

@Yaron10

The number of open documents is limited to 10.
If @donho approves, I don't see why it shouldn't be changed to 20.

I will consider it - could you create an issue for that please?

donho · 2022-02-17T11:52:57Z

@deebash
There are 2 missing stuffs:

Localization ability (that you have mentioned) + english.xml .
The possibility to add/change the shortcut of these command: https://github.com/notepad-plus-plus/notepad-plus-plus/blob/master/PowerEditor/src/Parameters.cpp#L65

You can do them in this PR or in the next PR, it's up to you.
If you do them in the next PR, please create a new branch for each PR:
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.
Ref: https://github.com/notepad-plus-plus/notepad-plus-plus/blob/master/CONTRIBUTING.md#guidelines-for-pull-requests

Thank you

deebash · 2022-02-17T12:09:39Z

@donho I'll do in this PR itself, I'll give commit in few hours. And thank you for the brief comments.

Yaron10 · 2022-02-17T13:38:20Z

@donho,

could you create an issue for that please?

#11229.

Refactoring: I would add a new method here with the following code in WindowsDlg.cpp

👍

Thank you.

1. Code Refactoring 2. Localization ability 3. Shortcut Mapper

donho

Add "Sort By" before all the entries of MainCommandNames in ShortcutMapper.

donho · 2022-02-20T15:47:06Z

+                    <Item id="11006" name="Type A to Z"/>
+                    <Item id="11007" name="Type Z to A"/>
+                    <Item id="11008" name="Size Smaller to Larger"/>
+                    <Item id="11009" name="Size Larger to Smaller"/>


Please add "Sort By" before all the entries. For example:

<Item id="11002" name="Sort By Name A to Z"/> <Item id="11003" name="Sort By Name Z to A"/> <Item id="11004" name="Sort By Path A to Z"/> <Item id="11005" name="Sort By Path Z to A"/> <Item id="11006" name="Sort By Type A to Z"/> ...

because these entries name will appear in the shortcut mapper without "Sort By" menu. With "Sort By" in front of each command, users won't be confused.

Sort By prefix added

deebash · 2022-02-20T16:53:52Z

@donho I added Sort By prefix before each entries. But changes are taking effect only after language change in preferences. Is this the actual behaviour? It's happening for all entries.

donho · 2022-02-20T17:43:51Z

@deebash

I added Sort By prefix before each entries. But changes are taking effect only after language change in preferences. Is this the actual behaviour?

What do you mean by "changes are taking effect only after language change in preferences." ?

donho · 2022-02-20T18:06:24Z

+	{ VK_NULL,    IDM_WINDOW_SORT_FT_DSC,                       false, false, false, nullptr },
+	{ VK_NULL,    IDM_WINDOW_SORT_FS_ASC,                       false, false, false, nullptr },
+	{ VK_NULL,    IDM_WINDOW_SORT_FS_DSC,                       false, false, false, nullptr },
+


changes are taking effect only after language change in preferences.

I guess I do understand what you said now - my bad, I forgot these entries.
I'll do a PR to fix them (and you will understand why this behaviour you describe above).

donho · 2022-02-20T19:24:49Z

+			sortTrans = nameW;
+			::ModifyMenu(menuHandle, 0, MF_BYPOSITION, 0, sortTrans.c_str());
+		}
+


Normally, it shouldn't be here this block.

Yaron10 · 2022-02-20T20:07:28Z

@deebash & @donho,

👍
Great work. Thank you.

Yaron10 · 2022-02-23T12:12:06Z

#11271.
#11272.

Added "Sort By" function under Window Menu

8bdd1b7

Added sorting function under Window Menu for issue #10393

mere-human reviewed Feb 12, 2022

View reviewed changes

chcg added the enhancement Proposed enhancements of existing features label Feb 13, 2022

donho self-assigned this Feb 16, 2022

donho requested changes Feb 16, 2022

View reviewed changes

Changes in Sort By function

bcd0180

3 changes mentioned by @mere-human done in this commit.

donho reviewed Feb 17, 2022

View reviewed changes

Indent error fix

88388af

Indent error fixed in this commit

donho requested changes Feb 17, 2022

View reviewed changes

donho reviewed Feb 17, 2022

View reviewed changes

Yaron10 mentioned this pull request Feb 17, 2022

Increase the number of open documents displayed in Window menu #11229

Closed

Localization & Shortcut Mapper

c7a861b

1. Code Refactoring 2. Localization ability 3. Shortcut Mapper

deebash requested a review from donho February 20, 2022 02:49

donho requested changes Feb 20, 2022

View reviewed changes

Sort By prefix added

f7fa578

Sort By prefix added

donho approved these changes Feb 20, 2022

View reviewed changes

donho added the accepted label Feb 20, 2022

donho closed this in 1c8b867 Feb 20, 2022

donho reviewed Feb 20, 2022

View reviewed changes

Conversation

deebash commented Feb 12, 2022

Uh oh!

Yaron10 commented Feb 12, 2022

Uh oh!

deebash commented Feb 12, 2022

Uh oh!

Yaron10 commented Feb 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deebash commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yaron10 commented Feb 14, 2022

Uh oh!

deebash commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yaron10 commented Feb 15, 2022

Uh oh!

deebash commented Feb 16, 2022

Uh oh!

donho left a comment

Choose a reason for hiding this comment

Uh oh!

deebash commented Feb 17, 2022

Uh oh!

mere-human commented Feb 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donho left a comment

Choose a reason for hiding this comment

Uh oh!

donho Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donho commented Feb 17, 2022

Uh oh!

donho commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deebash commented Feb 17, 2022

Uh oh!

Yaron10 commented Feb 17, 2022

Uh oh!

donho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deebash commented Feb 20, 2022

Uh oh!

donho commented Feb 20, 2022

Uh oh!

Choose a reason for hiding this comment

deebash commented Feb 13, 2022 •

edited

Loading

deebash commented Feb 15, 2022 •

edited

Loading

donho Feb 17, 2022 •

edited

Loading

donho commented Feb 17, 2022 •

edited

Loading