zulip-desktop icon indicating copy to clipboard operation
zulip-desktop copied to clipboard

Left sidebar: Added ordering feature for server tabs

Open Aitchessbee opened this issue 11 months ago • 6 comments

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
  • [x] Linux/Ubuntu
  • [ ] macOS

Aitchessbee avatar Mar 13 '24 13:03 Aitchessbee

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.

timabbott avatar Mar 13 '24 21:03 timabbott

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...

Aitchessbee avatar Mar 14 '24 08:03 Aitchessbee

Thoughts on https://github.com/zulip/zulip-desktop/pull/1059#issuecomment-812942418? Is the strategy for this PR that you're just doing a hard-reload of the app to avoid having to solve that problem?

timabbott avatar Mar 14 '24 17:03 timabbott

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?

Aitchessbee avatar Mar 14 '24 18:03 Aitchessbee

  • 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 avatar Mar 24 '24 19:03 Aitchessbee

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

Aitchessbee avatar Mar 28 '24 19:03 Aitchessbee