-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Desktop: Sort Order Buttons and Per-Notebook Sort Order #5437
Changes from 7 commits
e644195
bf100e3
91de06d
c723ebe
4cdba95
e9dcc3b
5d0b0e3
36e00f8
e75b50c
50342cb
6a4d64c
fd2d5f9
05e7846
b3cfbc8
057460e
9bef92d
8d75ae2
31cf9f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
import { CommandContext, CommandDeclaration, CommandRuntime } from '@joplin/lib/services/CommandService'; | ||
import Setting from '@joplin/lib/models/Setting'; | ||
import { _ } from '@joplin/lib/locale'; | ||
|
||
let fields: string[] = null; | ||
let perFieldReverse: { [field: string]: boolean } = null; | ||
|
||
export const notesSortOrderFieldArray = (): string[] => { | ||
// The order of the fields is strictly determinate. | ||
if (fields == null) { | ||
fields = Setting.enumOptionValues('notes.sortOrder.field').sort().reverse(); | ||
} | ||
return fields; | ||
}; | ||
|
||
export const notesSortOrderNextField = (currentField: string) => { | ||
const fields = notesSortOrderFieldArray(); | ||
const index = fields.indexOf(currentField); | ||
if (index < 0) { | ||
return currentField; | ||
} else { | ||
return fields[(index + 1) % fields.length]; | ||
} | ||
}; | ||
|
||
export const declaration: CommandDeclaration = { | ||
name: 'notesSortOrderSwitch', | ||
label: () => _('Switch sort order'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Toggle sort order There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finished. 36e00f8 |
||
parentLabel: () => _('Notes'), | ||
}; | ||
|
||
export const runtime = (): CommandRuntime => { | ||
return { | ||
execute: async (_context: CommandContext, field?: string | Array<any>, reverse?: boolean) => { | ||
// field: Sort order's field. undefined means switching a field. | ||
// reverse: whether the sort order is reversed or not. undefined means toggling. | ||
// | ||
// To support CommandService.scheduleExecute(), field accepts an size-two Array, | ||
// which means [field, reverse]. | ||
let nextField: string; | ||
let nextReverse: boolean; | ||
if (typeof field !== 'object') { | ||
nextField = field; | ||
nextReverse = reverse; | ||
} else { | ||
nextField = field[0]; | ||
nextReverse = field[1]; | ||
} | ||
const currentField = Setting.value('notes.sortOrder.field'); | ||
const currentReverse = Setting.value('notes.sortOrder.reverse'); | ||
const enabled = Setting.value('notes.perFieldReversalEnabled'); | ||
if (enabled) { | ||
if (perFieldReverse === null) { | ||
perFieldReverse = { ...Setting.value('notes.perFieldReverse') }; | ||
} | ||
} | ||
if (typeof field === 'undefined') { | ||
if (typeof reverse === 'undefined') { | ||
// If both arguments are undefined, the next field is selected. | ||
nextField = notesSortOrderNextField(currentField); | ||
} else { | ||
nextField = currentField; | ||
} | ||
} | ||
if (typeof reverse === 'undefined') { | ||
if (enabled && perFieldReverse.hasOwnProperty(nextField)) { | ||
nextReverse = !!perFieldReverse[nextField]; | ||
} else { | ||
nextReverse = currentReverse; | ||
} | ||
} | ||
if (currentField !== nextField) { | ||
Setting.setValue('notes.sortOrder.field', nextField); | ||
} | ||
if (currentReverse !== nextReverse) { | ||
Setting.setValue('notes.sortOrder.reverse', nextReverse); | ||
} | ||
if (enabled) { | ||
// nextField is sane here. | ||
nextReverse = !!nextReverse; | ||
if (perFieldReverse[nextField] !== nextReverse) { | ||
perFieldReverse[nextField] = nextReverse; | ||
Setting.setValue('notes.perFieldReverse', { ...perFieldReverse }); | ||
} | ||
} | ||
}, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import CommandService, { CommandContext, CommandDeclaration, CommandRuntime } from '@joplin/lib/services/CommandService'; | ||
import Setting from '@joplin/lib/models/Setting'; | ||
import { _ } from '@joplin/lib/locale'; | ||
|
||
export const declaration: CommandDeclaration = { | ||
name: 'notesSortOrderToggleReverse', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. toggleNoteSortOrderReverse There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finished. 36e00f8 |
||
label: () => Setting.settingMetadata('notes.sortOrder.reverse').label(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finished. 36e00f8 |
||
parentLabel: () => _('Notes'), | ||
}; | ||
|
||
export const runtime = (): CommandRuntime => { | ||
return { | ||
execute: async (_context: CommandContext) => { | ||
const reverse = Setting.value('notes.sortOrder.reverse'); | ||
return CommandService.instance().execute('notesSortOrderSwitch', undefined, !reverse); | ||
}, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,184 @@ | ||
import CommandService, { CommandContext, CommandDeclaration, CommandRuntime } from '@joplin/lib/services/CommandService'; | ||
import Setting from '@joplin/lib/models/Setting'; | ||
import eventManager from '@joplin/lib/eventManager'; | ||
import { notesSortOrderFieldArray } from './notesSortOrderSwitch'; | ||
import { _ } from '@joplin/lib/locale'; | ||
|
||
let previousFolderId: string = null; | ||
const folderState = { notesParentType: '', selectedFolderId: '' }; | ||
let ownSortOrders: { [key: string]: any } = null; | ||
const sharedSortOrder: { [key: string]: any } = { | ||
field: 'user_updated_time', | ||
reverse: true, | ||
user_updated_time: true, | ||
user_created_time: true, | ||
title: false, | ||
order: false, | ||
}; | ||
|
||
export function hasOwnSortOrder(folderId: string) { | ||
return folderId && ownSortOrders && ownSortOrders.hasOwnProperty(folderId + SUFFIX_FIELD); | ||
} | ||
|
||
export const declaration: CommandDeclaration = { | ||
name: 'perFolderSortOrder', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. togglePerFolderSortOrder There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finished. 36e00f8 |
||
label: () => _('Toggle own sort order'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Toggle per-notebook sort order" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finished. 36e00f8 |
||
}; | ||
|
||
export const runtime = (): CommandRuntime => { | ||
loadOwnSortOrders(); | ||
loadSharedSortOrder(); | ||
eventManager.appStateOn('notesParentType', onFolderSelectionMayChange.bind(null, 'notesParentType')); | ||
eventManager.appStateOn('selectedFolderId', onFolderSelectionMayChange.bind(null, 'selectedFolderId')); | ||
|
||
return { | ||
enabledCondition: 'oneFolderSelected', | ||
|
||
execute: async (context: CommandContext, folderId?: string, own?: boolean) => { | ||
let targetId = folderId; | ||
const selectedId = getSelectedFolderId(context.state); | ||
if (!targetId) { | ||
targetId = selectedId; // default: selected folder | ||
if (!targetId) return; | ||
} | ||
const targetOwn = hasOwnSortOrder(targetId); | ||
let newOwn; | ||
if (typeof own == 'undefined') { | ||
newOwn = !targetOwn; // default: toggling | ||
} else { | ||
newOwn = !!own; | ||
if (newOwn === targetOwn) return; | ||
} | ||
if (newOwn) { | ||
let field: string, reverse: boolean; | ||
if (!hasOwnSortOrder(selectedId)) { | ||
field = Setting.value('notes.sortOrder.field'); | ||
reverse = Setting.value('notes.sortOrder.reverse'); | ||
} else { | ||
field = sharedSortOrder.field; | ||
if (Setting.value('notes.perFieldReversalEnabled')) { | ||
reverse = sharedSortOrder[field]; | ||
} else { | ||
reverse = sharedSortOrder.reverse; | ||
} | ||
} | ||
setOwnSortOrder(targetId, field, reverse); | ||
} else { | ||
deleteOwnSortOrder(targetId); | ||
} | ||
}, | ||
}; | ||
}; | ||
|
||
function onFolderSelectionMayChange(cause: string, event: any) { | ||
if (cause !== 'notesParentType' && cause !== 'selectedFolderId') return; | ||
folderState[cause] = event.value; | ||
const selectedId = getSelectedFolderId(folderState); | ||
if (previousFolderId === null) previousFolderId = selectedId; | ||
if (previousFolderId === selectedId) return; | ||
const field = Setting.value('notes.sortOrder.field'); | ||
const reverse = Setting.value('notes.sortOrder.reverse'); | ||
const previousFolderHasOwnSortOrder = hasOwnSortOrder(previousFolderId); | ||
if (previousFolderHasOwnSortOrder) { | ||
setOwnSortOrder(previousFolderId, field, reverse); | ||
} else { | ||
setSharedSortOrder(field, reverse); | ||
} | ||
previousFolderId = selectedId; | ||
let next; | ||
if (hasOwnSortOrder(selectedId)) { | ||
next = getOwnSortOrder(selectedId); | ||
} else if (previousFolderHasOwnSortOrder) { | ||
next = sharedSortOrder; | ||
} else { | ||
return; | ||
} | ||
if (Setting.value('notes.perFolderSortOrderEnabled')) { | ||
if (next.field != field || next.reverse != reverse) { | ||
void CommandService.instance().execute('notesSortOrderSwitch', next.field, next.reverse); | ||
} | ||
} | ||
} | ||
|
||
const SUFFIX_FIELD = '$field'; | ||
const SUFFIX_REVERSE = '$reverse'; | ||
|
||
function getSelectedFolderId(state: { notesParentType: string; selectedFolderId: string }): string { | ||
if (state.notesParentType === 'Folder') { | ||
return state.selectedFolderId; | ||
} else { | ||
return ''; | ||
} | ||
} | ||
|
||
function getOwnSortOrder(folderId: string) { | ||
const field = ownSortOrders[folderId + SUFFIX_FIELD] as string; | ||
const reverse = ownSortOrders[folderId + SUFFIX_REVERSE] as boolean; | ||
return { field, reverse }; | ||
} | ||
|
||
function setOwnSortOrder(folderId: string, field: string, reverse: boolean) { | ||
const old = getOwnSortOrder(folderId); | ||
let dirty = false; | ||
if (old.field !== field) { | ||
ownSortOrders[folderId + SUFFIX_FIELD] = field; | ||
dirty = true; | ||
} | ||
if (old.reverse !== reverse) { | ||
ownSortOrders[folderId + SUFFIX_REVERSE] = reverse; | ||
dirty = true; | ||
} | ||
if (dirty) { | ||
Setting.setValue('notes.perFolderSortOrders', { ...ownSortOrders }); | ||
} | ||
} | ||
|
||
function deleteOwnSortOrder(folderId: string) { | ||
let dirty = false; | ||
if (ownSortOrders.hasOwnProperty(folderId + SUFFIX_FIELD)) { | ||
delete ownSortOrders[folderId + SUFFIX_FIELD]; | ||
dirty = true; | ||
} | ||
if (ownSortOrders.hasOwnProperty(folderId + SUFFIX_REVERSE)) { | ||
delete ownSortOrders[folderId + SUFFIX_REVERSE]; | ||
dirty = true; | ||
} | ||
if (dirty) { | ||
Setting.setValue('notes.perFolderSortOrders', { ...ownSortOrders }); | ||
} | ||
} | ||
|
||
function loadOwnSortOrders() { | ||
ownSortOrders = { ...Setting.value('notes.perFolderSortOrders') }; | ||
} | ||
|
||
function loadSharedSortOrder() { | ||
const validFields = notesSortOrderFieldArray(); | ||
const value = Setting.value('notes.sharedSortOrder'); | ||
for (const key in sharedSortOrder) { | ||
if (value.hasOwnProperty(key)) { | ||
if (key !== 'field' || validFields.includes(value.field)) { | ||
sharedSortOrder[key] = value[key]; | ||
} | ||
} | ||
} | ||
} | ||
|
||
function setSharedSortOrder(field: string, reverse: boolean) { | ||
let dirty = false; | ||
if (sharedSortOrder.field !== field) { | ||
sharedSortOrder.field = field; | ||
dirty = true; | ||
} | ||
if (sharedSortOrder.reverse !== reverse) { | ||
sharedSortOrder.reverse = reverse; | ||
dirty = true; | ||
} | ||
if (sharedSortOrder[field] !== reverse) { | ||
sharedSortOrder[field] = reverse; | ||
dirty = true; | ||
} | ||
if (dirty) { | ||
Setting.setValue('notes.sharedSortOrder', { ...sharedSortOrder }); | ||
} | ||
} |
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.
By the way commands are normally named as
verbNoun
and we use the word "toggle" usually. So it should be name "toggleNoteSortOrder" for consistency.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.
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.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.
Finished. 36e00f8
Command names are changed as below.