Desktop: Sort Order Buttons and Per-Notebook Sort Order

[ken1kob]

The features discussed at the below are implemented. The detailed features and their animation examples are described at the first post of the topic.

• Sort Order Buttons and Per-Notebook Sort Order - Development - Joplin Forum

[laurent22]

Thanks for the pull request. Since the thread if many pages long, could you provide a summary of what's been implemented please?

[ken1kob]

could you provide a summary of what's been implemented please?

Very short summary is here. The following three features are implemented.

1. Sort Order Buttons
   • Two buttons, "Switch sort order field" and "Reverse sort order", are added at the top of NoteListPanel.
2. Per-Field Reversal
   • Normal/Reverse order is kept for each sort order field (updated time, created time, title or cutom order).
3. Per-Notebook Sort Order
   • Some notebooks can have their own sort orders (field + reverse) for their notes.

For more details, it is enough to read "Detailed Features" in the 1st post.

[laurent22]

Thanks for the information. Is the post on the forum up to date? I'm asking these questions because at first glance the code looks fine, so what would need to be discussed is what's implemented, and it starts with a detailed spec (which you seem to have already provided but just want to make sure it's up to date).

[laurent22]

Also you have several linter errors: https://github.com/laurent22/joplin/pull/5437/checks?check_run_id=3544976159#step:8:600

It seems many developers are having this problem lately. Is the pre-commit hook not working for you? When committing do you see something like this?

[ken1kob]

Thanks for the information. Is the post on the forum up to date?

Yes. In the topic, there are some discussions including new coding (such as adding a context menu). But, at the end, the initial feature details become the final ones.

[ken1kob]

Also you have several linter errors: https://github.com/laurent22/joplin/pull/5437/checks?check_run_id=3544976159#step:8:600

Sorry. They are fixed in the commit a while ago.

It seems many developers are having this problem lately. Is the pre-commit hook not working for you? When committing do you see something like this?

Husky does not seem to work even though it is successfully installed. (My env is Win10 + powershell/cygwin. )

> git commit -m "test" .
Can't find Husky, skipping pre-commit hook
You can reinstall it using 'npm install husky --save-dev' or delete this hook

Usually, I manually run eslint by npm run linter-ci .. In this time, I forgot to put the last period, and running npm run linter-ci reports nothing.

[ken1kob]

But, at the end, the initial feature details become the final ones.

Though the features are not changed, icons are changed as shown at the post.

[laurent22]

I've add a first look at it. The implementation is good overall, it's great in particular that you've packaged your feature as commands as it makes the whole thing very flexible. I appreciate too the detailed spec and rational in the forum, as this important part is often missing in pull requests.

Beside the comments I left, I would have the following remarks:

• Once a folder has had custom sort settings, is it possible to revert to the default? i.e. to reset the custom state and start using the default again?

• Currently it's already possible to manually order the notes (via the notes.order field), and that order is then synced to all devices including mobile. As I understand your feature will not be synced and I wonder if it should? It might be tricky but I think people will setup their notebooks as they like, then switch to a different device, and nothing will be synced, so they'll have to do that setup again.

• Once the feature is stable, we'll need to have some test units for it.

[laurent22]

In code, we use the term "folder", not "notebook", so please change to onFolderSelectionMayChange

[ken1kob]

I see.

[ken1kob]

finished. c723ebe

[laurent22]

onFolderSelectionMayChange

[ken1kob]

finished. c723ebe

[laurent22]

getSelectedFolderId

[ken1kob]

finished. c723ebe

[laurent22]

previousFolderHasOwnSortOrder

There are a few others so please go through the code and rename to "folder" every time. It's something I'm trying to keep consistent even though there's an inconsistency between UI (notebook) and backend (folder).

[ken1kob]

finished. c723ebe

[laurent22]

I don't think we need this layer of indirection. In general, the code should just get the setting directly, without going through a function (unless of course the function does additional processing, but that's not happening in this case). Although I realise you lose typing in this case, but then we should probably change Setting.value to a Setting.value<T>

[ken1kob]

I see.

[ken1kob]

finished. 4cdba95

[ken1kob]

• Once a folder has had custom sort settings, is it possible to revert to the default? i.e. to reset the custom state and start using the default again?

Yes. It can be toggled by the context menu in the Side Bar (toggling) and perNotebookFolderSortOrder command. Besides, the current state is shown as a check mark in the Side Bar's context menu.

• Currently it's already possible to manually order the notes (via the notes.order field), and that order is then synced to all devices including mobile. As I understand your feature will not be synced and I wonder if it should? It might be tricky but I think people will setup their notebooks as they like, then switch to a different device, and nothing will be synced, so they'll have to do that setup again.

Yes. I think it should be synced. But, since implementing the feature of synchronizing them has some impact, it would be postponed until people wants it. As a temporary workaround, some of settings are stored in a file (not a db) for advanced users to be ported manually.

• Once the feature is stable, we'll need to have some test units for it.

I see. Both GUI and non-GUI? If you pick up some existing tests, I'll follow their manner.

[laurent22]

I had another look at it and added a few more comments. The one thing I can't find, and I've looked several times, is where you actually sort the notes? I'm sure it's there but somehow can't see it. Could you point me to the relevant lines please?

[laurent22]

Ideally we should never call getState() directly - instead you should only pass the state to the function.

[ken1kob]

finished. 5d0b0e3

[laurent22]

getState() should be removed - instead the variables you need should be passed to the component using Redux.

[ken1kob]

finished. e9dcc3b

[laurent22]

Also don't call Setting.value() from components, because if the value is changed the component should be re-rendered, but it won't be when using Setting.value. Instead pass the values you need to the props using Redux connect, and the state - state.settings['notes.sortOrder.buttonsVisible']

[ken1kob]

As a come-lately committer, I felt reluctant to touch React component's state and props when not needed. If you don't mind, it would be really better.

By the way, since NoteListControl changes when any 'sort.noteOrderXXX' setting changes, the code in this commit works properly as expected.

[ken1kob]

finished. e9dcc3b

[laurent22]

not calling Setting.value

[ken1kob]

finished. e9dcc3b

[laurent22]

Please don't use constants for setting name as it's another layer of indirection, just use the string directly.

[ken1kob]

finished. 4cdba95

[laurent22]

Also no constant please.

[ken1kob]

finished. 4cdba95

[laurent22]

This should not be here - there has to be some existing code that does something when folder selection change, so please move it over there. That way, you also won't need to include the app package in the comment (creates a dependency loop).

[ken1kob]

First, the dependency on 'app' could be removed from this command without any redesign. 5d0b0e3

Before we proceed further, I want to clarify your intentions.

This should not be here - there has to be some existing code that does something when folder selection change, so please move it over there.

Which is your suggestion intend, only removing dependency on app or that such a functionality should be moved toward the core?

If the former, everything is done.

If the latter, I have some questions. The first is where is it moved to? Around lib/reducer.ts, or around app-desktop/app.reducer.ts, or in some React component (maybe, NoteListXXX)? I'm not sure the features play a role in the core rather than app-desktop.

Another question is how to capture changes of states/settings. First, modifying lib/reducer.ts is definite and effective, but it will worsen a certain degree of the maintainability, because changing codes of folerSelectionId and notesParentType are scattered. In another side, I don't have a good solution to modify app-desktop/app.reducer.ts. Finally, componentDidUpdated(), useEffect() and appStateOn() are handy to implement the feature.

[laurent22]

In fact, most of these functions should be moved to a utils package. Could you move them under maybe /services/perFolderSortOrder/utils.ts? Unless you can find a better place.

[ken1kob]

Migrating to more appropriate venues is very fine.

One suggestion is that most part of perFolderSortOrder.ts and notesSortOrderSwitch.ts are moved to the followings respectively.

• app-desktop/services/perFolderSortOrderUtils.ts
• app-desktop/services/notesSortOrderUtils.ts

[laurent22]

I would prefer if those related functions are grouped so either in separate files but in a new folder, or both in the same file.

[ken1kob]

I see. Final locations are:

• app-desktop/services/sortOrder/notesSortOrderUtils.ts
• app-desktop/services/sortOrder/PerFolderSortOrderService.ts

finished. 36e00f8

[ken1kob]

The one thing I can't find, and I've looked several times, is where you actually sort the notes? I'm sure it's there but somehow can't see it. Could you point me to the relevant lines please?

• Answer 1)

  • NoteListComponent's props has noteSortOrder: state.settings['notes.sortOrder.field'].

• Answer 2)

  • The change of 'notes.sortOrderXXX' settings always invoke BaseApplication.refreshNotes()
    

    joplin/packages/lib/BaseApplication.ts

    Lines 538 to 540 in 6d98186

    	if (this.hasGui() && ((action.type == 'SETTING_UPDATE_ONE' && action.key.indexOf('notes.sortOrder') === 0) || action.type == 'SETTING_UPDATE_ALL')) {
    	refreshNotes = true;
    	}

The change of notes.sortOrder.field is captured by the both. But, the change of notes.sortOrder.reverse is captured by the latter only.

[laurent22]

By the way commands are normally named as verbNoun and we use the word "toggle" usually. So it should be name "toggleNoteSortOrder" for consistency.

[ken1kob]

I see. I have no objection to verbNoun style and other naming conventions. Speaking my preferences, "toggle" would be limited to switching between two states, since its behavior is very self-explanatory. For three or more states, I prefer other verbs, switch, select, change, or cycle.

[ken1kob]

Finished. 36e00f8
Command names are changed as below.

• notesSortOrderSwitch -> toggleNotesSortOrderField
• notesSortOrderToggleReverse -> toggleNotesSortOrderReverse
• perFolderSortOrderSwitch -> togglePerFolderSortOrder

[laurent22]

Toggle sort order

[ken1kob]

Finished. 36e00f8

[laurent22]

toggleNoteSortOrderReverse

[ken1kob]

Finished. 36e00f8

[laurent22]

That's sensible, but we shouldn't actually access Settings from the declaration. They are probably initialised by then we there's no guarantee. So you'll just need to copy and paste the string.

[ken1kob]

I see.

[ken1kob]

Finished. 36e00f8

[laurent22]

togglePerFolderSortOrder

[ken1kob]

Finished. 36e00f8

[laurent22]

"Toggle per-notebook sort order"

[ken1kob]

Finished. 36e00f8

[laurent22]

The change of notes.sortOrder.field is captured by the both. But, the change of notes.sortOrder.reverse is captured by the latter only.

Thanks for the explanation, that makes sense. I'm a bit concerned with this change as I intend to update the note list at some point, but it seems your changes are well isolated and shouldn't make refactoring more complex.

[laurent22]

Please define interfaces for these.

[ken1kob]

finished. 00bc482

[laurent22]

All methods should be explicitely marked as either "public" or "private". I see you've marked the private ones already and I guess you intend the others to be public by default, but I prefer if it's explicit (in fact you probably got a few warnings about this with eslint).

[ken1kob]

finished. 00bc482

[laurent22]

Please define the output type

[laurent22]

strict equality

[ken1kob]

finished. 00bc482

[laurent22]

Thank you for refactoring this into a class. Now that it's separate I guess it should be easier to unit test it too.

[laurent22]

I don't know if it was due to my recent changes to the style, but the sort button is actually too tall:

Also the "A" button shoudl be the same size as the "New note" button on the right. The "arrow" part should have the same padding as the other buttons. So it means that the whole button will be wider.

Also I wonder if there should be an option to hide or show this button? I feel it's getting a bit cluttered in this area. But don't implement anything yet, perhaps ask on the forum what users think?

[ken1kob]

I don't know if it was due to my recent changes to the style, but the sort button is actually too tall:

Yes. It's affected by the recent change. I'll adapt them to v2.5.

It'll become as below.

Also I wonder if there should be an option to hide or show this button? I feel it's getting a bit cluttered in this area. But don't implement anything yet, perhaps ask on the forum what users think?

It's already implemented as in Detailed Features.

[laurent22]

Yes. It's affected by the recent change. I'll adapt them to v2.5.

Yes thank you, it looks good like this. I think you can use size={ButtonSize.Small} to make it the right height.

It's already implemented as in Detailed Features.

Somehow I checked in the Appearance section but not the other ones. In fact I wonder if the Note section is right for this, since it's more about the notebooks? Perhaps create a new "Notebook" section for it?

Also could you please add a description for each field (in the Setting "description" field)?

Do we need the "Enable per-field sort order reversal" option actually? This is a very useful feature you've added there, and I think it could simply be on by default. In general there's no logical reason to always prefer reversal or always normal order - it depends, as you noted, on the field. So that's why I think your option could always be on by default.

[ken1kob]

Somehow I checked in the Appearance section but not the other ones. In fact I wonder if the Note section is right for this, since it's more about the notebooks? Perhaps create a new "Notebook" section for it?

Yes. I think Appearance is more appropriate for sort buttons, and Notebook is more suitable for the per-folder feature.
At first, I was very conservative and refrained from creating a new section just for this PR.

Also could you please add a description for each field (in the Setting "description" field)?

I see.

Do we need the "Enable per-field sort order reversal" option actually? This is a very useful feature you've added there, and I think it could simply be on by default.

I think so. I'll hide it from the option menu in the next commit.

[ken1kob]

Yes. It's affected by the recent change. I'll adapt them to v2.5.

Yes thank you, it looks good like this. I think you can use size={ButtonSize.Small} to make it the right height.

Yes. I think Appearance is more appropriate for sort buttons, and Notebook is more suitable for the per-folder feature.

Also could you please add a description for each field (in the Setting "description" field)?

I think so. I'll hide it from the option menu in the next commit.

finished. 00bc482

[laurent22]

Thanks for the update @ken1kob.

Could you also get my commit there please?1ac8265
It is to fix the button size as it was growing and shrinking depending on the icon.

Also besides the comments I left below, could you add test units for PerFolderSortOrderSerice and notesSortOrderUtils please? You just need to test the public methods (which should indirectly test the private ones). If you need some help for this let me know, but you can check any of the ".test.ts" files to see how to set it up.

[laurent22]

If the setting is no longer public, the description should be removed (to avoid unecessary translations).

[ken1kob]

Finished. 54de991

[laurent22]

Shoudl be called "folder" (Always "folder" in code, always "notebook" in user strings).

[ken1kob]

Finished. 54de991

[laurent22]

"folder"

[ken1kob]

Finished. 54de991

[laurent22]

"per-notebook".

But also, when the per-notebook option is not enabled on the notebook, this is actually like a global setting that applies not, per-notebook, but to all notebooks, is that correct?

I don't think we can convey all that info in this tooltip, so simply "Toggle sort order" would be sufficient (it's implied it's for the notebook).

[ken1kob]

But also, when the per-notebook option is not enabled on the notebook, this is actually like a global setting that applies not, per-notebook, but to all notebooks, is that correct?

When notes.perFolderSortOrderEnabled is false, this command has no effect, and it is as expected.

I don't think we can convey all that info in this tooltip, so simply "Toggle sort order" would be sufficient (it's implied it's for the notebook).

Since this feature is not very common, I'm afraid that too brevity introduces misunderstanding such as 'toggle sort order field (title/update time/create time/custom)'. I agree that "Toggle per-notebook sort order" is too long. How about "Toggle own sort order"?

[ken1kob]

Could you also get my commit there please? 1ac8265
It is to fix the button size as it was growing and shrinking depending on the icon.

Finished.

Also besides the comments I left below, could you add test units for PerFolderSortOrderSerice and notesSortOrderUtils please?

Finished. 903e000

[laurent22]

Thanks @ken1kob but you've accidentally included unrelated commits in your latest change, so please remove these.

[laurent22]

So the latest remaining issue is this one I guess: #5437 (comment) (putting it here so that's it not lost in the commits)

[ken1kob]

Thanks @ken1kob but you've accidentally included unrelated commits in your latest change, so please remove these.

Oops..., sorry. Fixed.

So the latest remaining issue is this one I guess: #5437 (comment) (putting it here so that's it not lost in the commits)

Yes. What's your favorite?

• "Toggle sort order"
• "Toggle per-notebook sort order"
• "Toggle own sort order"

[laurent22]

To sum up, if I understand correctly, now the global option "perFolderSortOrderEnabled" does nothing since it's always enabled and cannot be disabled. Instead, users can opt-in if they want the per-notebook behaviour or not. If they don't opt-in, it uses the global settings. Is that correct?

I think just "Toggle sort order" is fine because it's hard to convey exactly what the button does in the tooltip. "Toggle own sort order" would not even be accurate if the "per-notebook" setting is not enabled for the notebook anyway, would it? It would actually be "Toggle global sort order", which means we'd need two different tooltips depending on the notebook state, but I don't think the command system currently supports this.

[ken1kob]

To sum up, if I understand correctly, now the global option "perFolderSortOrderEnabled" does nothing since it's always enabled and cannot be disabled.

Yes. Since I'm not sure that this new UI feature is accepted by everyone at this time, I would like to keep perFolderSortOrderEnabled setting for a while, even if it is enabled and has no UI to disable it. Advanced users can still enable/disable it through directly editing settings.json.
But, I'm not sticking to it.

Instead, users can opt-in if they want the per-notebook behaviour or not. If they don't opt-in, it uses the global settings. Is that correct?

Yes. I'll summarize here. For each context menu of a notebook in SideBar,

Setting perFolderSortOrderEnabled	Context menu item ("Toggle per-notebook sort order")	Used sort order
true	enabled	the notebook's own sort order
true	disabled	global (=shared) sort order
false	N/A (not displayed)	global (=shared) sort order

I think just "Toggle sort order" is fine because it's hard to convey exactly what the button does in the tooltip. "Toggle own sort order" would not even be accurate if the "per-notebook" setting is not enabled for the notebook anyway, would it? It would actually be "Toggle global sort order", which means we'd need two different tooltips depending on the notebook state, but I don't think the command system currently supports this.

Perhaps there is a misunderstanding. The context menu item is not for specifying sort order field and reverse, but for specifying only whether it uses its own sort order or global one. If perFolderSortOrderEnabled is false, the specification is meaningless, and thus, the context menu item is not displayed in the context menu.

In any case, I have no strong opinion among the three naming options and will accept your choice.

[laurent22]

Perhaps there is a misunderstanding. The context menu item is not for specifying sort order field and reverse, but for specifying only whether it uses its own sort order or global one.

Yes indeed I misunderstood - for some reason I thought it was the button tooltip, but since it's for the context menu item it's different. So you're right, "Toggle own sort order" would make sense for this. If you could just update this I think it's ready and we can add the feature for 2.6.

[ken1kob]

If you could just update this I think it's ready and we can add the feature for 2.6.

Finished.

[laurent22]

Brilliant, it's all good then. Many thanks for adding this very useful feature @ken1kob!

[laurent22]

Now that I'm trying the feature I realise you were right about the toolbar button tooltip. Having something like "Toggle sort order: Creation date" for example would help to know what each icon means.

[ken1kob]

Shall I revert back to the old commit?

Ref: https://discourse.joplinapp.org/t/sort-order-buttons-and-per-notebook-sort-order/20013/39?u=ken1kob

[laurent22]

Yes that would be great if you could do that. Maybe in a new pull request?