Fix flashing icons of servers in sidebar

[kanishk98]

What's this PR do?

Changes order in which servers are loaded and rendered in the sidebar. I'd like to stop doing the latter half by retaining the server tabs

Any background context you want to provide?
#551 should provide the required context.
I've changed the order of loading servers, but rendering items in the sidebar according to the old order is turning out to be a problem (my understanding of how initServer() works is faulty here. Does it not render this.tabs in app/renderer/js/main.js according to the order in which the elements are present in the array?)

This problem is similar to the one I'm currently facing in https://github.com/zulip/zulip-electron/pull/617.

Screenshots? (according to zulip/zulip-electron@3ec20ca, last updated on Jan 7)

The refresh you see has been forced by me.

You have tested this PR on:

• Windows
• Linux/Ubuntu
• macOS

[abhigyank]

my understanding of how initServer() works is faulty here. Does it not render this.tabs in app/renderer/js/main.js according to the order in which the elements are present in the array?

It should call initServer in the order in which the domains.json saves the file.

I am not too sure if the changes you have done would lead us a good final solution. This part to implement is a bit tricky keeping in mind the user experience. You could try experimenting a bit on this. Understanding the function call orders with indexes should definitely help.

[abhigyank]

@kanishk98 I don't think your code logic is correct. For me refreshing, causes me to lose a server. Clicking on 2nd tab opens the 3rd tab, the order of tabs changes on its own on refreshing.

[kanishk98]

@abhigyank thanks for the feedback. I'll see what might've caused you to lose a server and change the click behaviour.

As for changing the order of tabs, I'm loading the last active tab as the first server in the list on every refresh. I assumed that we'd want that to be the first tab. Is that what you mean by the incorrect order?

[abhigyank]

I assumed that we'd want that to be the first tab. Is that what you mean by the incorrect order?

I think we'd want that to the be the first tab to load but not necessarily the first tab in the list on the left sidebar. The order on the sidebar should remain same but only the loading order could vary. @akashnimare Feel free to weigh in here.

[kanishk98]

@abhigyank What I understand from the previous comments is that we'd want to initialize the servers in the sidebar as per their original order, but the actual loading of their webviews happens only after the last active tab has been activated. If that's the case, I've changed my initTabs() to the following:

initTabs() {
		const servers = DomainUtil.getDomains();
		if (servers.length > 0) {
			for (let i = 0; i < servers.length; i++) {
				this.initServer(servers[i], i);
				DomainUtil.updateSavedServer(servers[i].url, i);
			}
			// activate last active tab and load its webview first
			const lastActiveTab = ConfigUtil.getConfigItem('lastActiveTab');
			this.activateTab(lastActiveTab);
			for (let i = 0; i < servers.length; i++) {
				if (i === lastActiveTab) {
					continue;
				}
				this.tabs[i].loadWebview();
			}
			// Remove focus from the settings icon at sidebar bottom
			this.$settingsButton.classList.remove('active');
		} else {
			this.openSettings('AddServer');
		}
	}

If you think I've got the correct idea now, I'll push this code.

[kanishk98]

@akashnimare @abhigyank does this PR look good?

Edit: I'm guessing the work on this PR is done and waiting review. I'll remove the WIP tag; please let me know if there is something else that needs to be done.

[abhigyank]

Need some time to review this :)

[akashnimare]

@kanishk98 can you fix the merge conflicts? Also, the desired behaviour should be -

• Load the last active server first and focus the same in the left sidebar
• Load all the rest servers in the background + don't focus on the tabs

[kanishk98]

Updated this PR's logic and also fixed the merge conflicts.
@akashnimare please let me know what you think whenever you're free.

[akashnimare]

@kanishk98 can we make the code cleaner a bit, maybe something like this -

let lastActiveTab = ConfigUtil.getConfigItem('lastActiveTab');

if (lastActiveTab >= servers.length) {
	lastActiveTab = 0;
}

// Following is the logic to handle which webview we need to load first
for (let i = 0; i < servers.length; i++) {
	if (i === lastActiveTab) {
		// load last active tab first
		DomainUtil.updateSavedServer(servers[lastActiveTab].url, lastActiveTab);
		this.activateTab(lastActiveTab);
		continue;
	}
	// load other webviews
	DomainUtil.updateSavedServer(servers[i].url, i);
	this.tabs[i].webview.load();
}

[kanishk98]

@akashnimare wouldn't the above snippet load them in order anyway? We would activate only the last active server, yes, but don't we also want to load that one first?

[akashnimare]

Merged. Thanks, @kanishk98.