Left sidebar: Added ordering feature for server tabs #1359

Aitchessbee · 2024-03-13T13:35:33Z

What's this PR do?
fixes #548
Ordering feature for server tabs using drag and drop

Libraries added - SortableJS, @types/sortablejs

You have tested this PR on:

Windows
Linux/Ubuntu
macOS

timabbott · 2024-03-13T21:37:40Z

Can you clean up this PR? Check out the Zulip commit guidelines for more details, but also please take the time to read our contributing guidelines and advice for creating reviewable pull requests.

Aitchessbee · 2024-03-14T08:27:15Z

Can you clean up this PR? Check out the Zulip commit guidelines for more details, but also please take the time to read our contributing guidelines and advice for creating reviewable pull requests.

Updated it... Let me know if any other changes are required...

timabbott · 2024-03-14T17:41:30Z

Thoughts on #1059 (comment)? Is the strategy for this PR that you're just doing a hard-reload of the app to avoid having to solve that problem?

Aitchessbee · 2024-03-14T18:31:13Z

Yeah! Server reordering is not something a user is not going to be doing very often, and given the complications of the index issues we'll be facing through other approaches, I found hard reloading to be the best way to ensure everything works properly. What are your thoughts on this?

andersk

I’m seeing an issue where horizontal scrolling gets triggered during drag (maybe by a tooltip?).

Repeatedly hard-reloading the app seems to mildly annoy Electron:

(node:807197) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 render-view-deleted listeners added to [WebContents]. Use emitter.setMaxListeners() to increase limit
    at _addListener (node:events:591:17)
    at WebContents.addListener (node:events:609:10)
    at ObjectsRegistry.registerDeleteListener (/home/anders/zulip/zulip-desktop/dist-electron/index.js:190:17)
    at ObjectsRegistry.add (/home/anders/zulip/zulip-desktop/dist-electron/index.js:115:12)
    at valueToMeta (/home/anders/zulip/zulip-desktop/dist-electron/index.js:431:40)
    at /home/anders/zulip/zulip-desktop/dist-electron/index.js:655:14
    at IpcMainImpl.<anonymous> (/home/anders/zulip/zulip-desktop/dist-electron/index.js:585:23)
    at IpcMainImpl.emit (node:events:517:28)
    at IpcMainImpl.emit (node:domain:489:12)

andersk · 2024-03-21T23:28:05Z

app/renderer/js/main.ts

@@ -81,6 +82,7 @@ export class ServerManagerView {
  tabIndex: number;
  presetOrgs: string[];
  preferenceView?: PreferenceView;
+  sortableSidebar: SortableJS | null;


Suggested change

sortableSidebar: SortableJS | null;

sortableSidebar?: SortableJS;

to avoid explicitly assigning null.

andersk · 2024-03-21T23:28:13Z

app/renderer/js/main.ts

+      animation: 150,
+      onEnd: (event: SortableJS.SortableEvent) => {
+        // Update the domain order in the database
+        DomainUtil.updateDomainOrder(event.oldIndex ?? 0, event.newIndex ?? 0);


Unexpected data should be rejected and ignored, not silently replaced with fake values like 0.

andersk · 2024-03-21T23:28:39Z

app/renderer/js/utils/domain-util.ts

+  const domains = serverConfSchema
+    .array()
+    .parse(db.getObject<unknown>("/domains"));


Why not use getDomains()?

andersk · 2024-03-21T23:29:40Z

app/renderer/js/utils/domain-util.ts

+  const [movedDomain] = domains.splice(oldIndex, 1);
+  domains.splice(newIndex, 0, movedDomain);


If the database is out of sync (e.g. it has been manually edited), it’s possible for oldIndex and newIndex to be out of range. We should check for that.

andersk · 2024-03-21T23:30:19Z

app/renderer/js/utils/domain-util.ts

+  for (const [index, domain] of domains.entries()) {
+    updateDomain(index, domain);
+  }


Do this in a single db.push call, not one call per domain.

andersk · 2024-03-21T23:30:34Z

package.json

+    "@types/sortablejs": "^1.15.8",
+    "gatemaker": "https://github.com/andersk/gatemaker/archive/d31890ae1cb293faabcb1e4e465c673458f6eed2.tar.gz",
+    "sortablejs": "^1.15.2"


sortablejs and @types/sortablejs should go in devDependencies as they are bundled by Vite. (gatemaker is a weird exception due to the native library it uses on macOS.)

Aitchessbee · 2024-03-24T19:24:10Z

I’m seeing an issue where horizontal scrolling gets triggered during drag (maybe by a tooltip?).

Repeatedly hard-reloading the app seems to mildly annoy Electron:

(node:807197) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 render-view-deleted listeners added to [WebContents]. Use emitter.setMaxListeners() to increase limit
    at _addListener (node:events:591:17)
    at WebContents.addListener (node:events:609:10)
    at ObjectsRegistry.registerDeleteListener (/home/anders/zulip/zulip-desktop/dist-electron/index.js:190:17)
    at ObjectsRegistry.add (/home/anders/zulip/zulip-desktop/dist-electron/index.js:115:12)
    at valueToMeta (/home/anders/zulip/zulip-desktop/dist-electron/index.js:431:40)
    at /home/anders/zulip/zulip-desktop/dist-electron/index.js:655:14
    at IpcMainImpl.<anonymous> (/home/anders/zulip/zulip-desktop/dist-electron/index.js:585:23)
    at IpcMainImpl.emit (node:events:517:28)
    at IpcMainImpl.emit (node:domain:489:12)

Made all the changes... not sure what can be done about the warning that you are getting, Any ideas?

Aitchessbee · 2024-03-28T19:14:05Z

I think this is good to release, as the test is showing a false negative...

jni · 2024-10-28T13:36:11Z

Hello Zulip team! As per usual, thank you so much for Zulip! Actually Zulip itself brought me here — thanks to the awesome organisation I was able to find recent topics discussing this feature and find my way to this PR, saving noise and duplication for everyone. 🥳

It seems like this PR is ready for another round of review?

alya · 2024-10-30T17:04:04Z

@Aitchessbee Please clean up your commit history and post again to request a review. See here for guidelines.

You will also need to resolve conflicts.

zulipbot · 2024-12-11T19:21:45Z

Heads up @Aitchessbee, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added the size: L label Mar 13, 2024

Aitchessbee force-pushed the feature/sidebar-orgs-ordering branch from 875e22d to 7879f70 Compare March 13, 2024 13:38

left sidebar: added ordering feature for server tabs, fixes zulip#548

aedab17

Aitchessbee force-pushed the feature/sidebar-orgs-ordering branch from 1ce8837 to aedab17 Compare March 14, 2024 08:25

Aitchessbee changed the title ~~Added ordering feature for server tabs~~ Left sidebar: Added ordering feature for server tabs - fixes #548 Mar 14, 2024

Aitchessbee changed the title ~~Left sidebar: Added ordering feature for server tabs - fixes #548~~ Left sidebar: Added ordering feature for server tabs Mar 14, 2024

andersk reviewed Mar 21, 2024

View reviewed changes

Aitchessbee force-pushed the feature/sidebar-orgs-ordering branch 2 times, most recently from ec8ecc9 to 12ad3a2 Compare March 24, 2024 19:35

Aitchessbee force-pushed the feature/sidebar-orgs-ordering branch from 12ad3a2 to 21cfd2c Compare April 14, 2024 09:39

Aitchessbee force-pushed the feature/sidebar-orgs-ordering branch from 21cfd2c to a058716 Compare April 23, 2024 09:51

fixed overflow, redundant db calls, package.json

64e827a

Aitchessbee force-pushed the feature/sidebar-orgs-ordering branch from a058716 to 64e827a Compare April 23, 2024 09:54

zulipbot added the has conflicts label Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Left sidebar: Added ordering feature for server tabs #1359

Left sidebar: Added ordering feature for server tabs #1359

Aitchessbee commented Mar 13, 2024 •

edited

Loading

timabbott commented Mar 13, 2024

Aitchessbee commented Mar 14, 2024

timabbott commented Mar 14, 2024

Aitchessbee commented Mar 14, 2024

andersk left a comment

andersk Mar 21, 2024

andersk Mar 21, 2024

andersk Mar 21, 2024

andersk Mar 21, 2024

andersk Mar 21, 2024

andersk Mar 21, 2024

Aitchessbee commented Mar 24, 2024

Aitchessbee commented Mar 28, 2024

jni commented Oct 28, 2024

alya commented Oct 30, 2024

zulipbot commented Dec 11, 2024

	sortableSidebar: SortableJS \| null;
	sortableSidebar?: SortableJS;

		const [movedDomain] = domains.splice(oldIndex, 1);
		domains.splice(newIndex, 0, movedDomain);

Left sidebar: Added ordering feature for server tabs #1359

Are you sure you want to change the base?

Left sidebar: Added ordering feature for server tabs #1359

Conversation

Aitchessbee commented Mar 13, 2024 • edited Loading

timabbott commented Mar 13, 2024

Aitchessbee commented Mar 14, 2024

timabbott commented Mar 14, 2024

Aitchessbee commented Mar 14, 2024

andersk left a comment

Choose a reason for hiding this comment

andersk Mar 21, 2024

Choose a reason for hiding this comment

andersk Mar 21, 2024

Choose a reason for hiding this comment

andersk Mar 21, 2024

Choose a reason for hiding this comment

andersk Mar 21, 2024

Choose a reason for hiding this comment

andersk Mar 21, 2024

Choose a reason for hiding this comment

andersk Mar 21, 2024

Choose a reason for hiding this comment

Aitchessbee commented Mar 24, 2024

Aitchessbee commented Mar 28, 2024

jni commented Oct 28, 2024

alya commented Oct 30, 2024

zulipbot commented Dec 11, 2024

Aitchessbee commented Mar 13, 2024 •

edited

Loading