Skip to content

settings: Always show the sidebar in case of settings page. #1431

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

settings: Always show the sidebar in case of settings page. #1431

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions app/renderer/js/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,13 @@ export class ServerManagerView {
});
this.$settingsButton.classList.add("active");
this.preferenceView!.handleNavigation(navigationItem);

// We need to call toggleSidebar again since we always want to show
// the sidebar in case of the settings page. We still pass the real
// config value for showSidebar so we do not reset showSidebar
// if it was set to false by the user.
const showSidebar = ConfigUtil.getConfigItem("showSidebar", true);
this.toggleSidebar(showSidebar);
}

async openAbout(): Promise<void> {
Expand Down Expand Up @@ -712,6 +719,12 @@ export class ServerManagerView {
this.loading.has((await tab.webview).properties.url),
);

// We might be coming here from the settings page, which has the
// sidebar always enabled. We need to recheck that setting here
// and toggle the sidebar as needed.
const showSidebar = ConfigUtil.getConfigItem("showSidebar", true);
this.toggleSidebar(showSidebar);

ipcRenderer.send("update-menu", {
// JSON stringify this.tabs to avoid a crash
// util.inspect is being used to handle circular references
Expand Down Expand Up @@ -791,6 +804,12 @@ export class ServerManagerView {
}

toggleSidebar(show: boolean): void {
ConfigUtil.setConfigItem("showSidebar", show);
// We do not want to hide the sidebar in case of the settings page.
if (this.tabs[this.activeTabIndex]?.properties?.page === "Settings") {
show = true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it appears this is changing the behavior to now write to the settings (the default of whether to show the sidebar on startup) when toggling the sidebar manually. I don't think that's what we were looking to do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if that is the case i.e. the default of whether to show the sidebar is changed by setConfigItem.

From my understanding, it does not override any global config if override is not set to true, which it is not in that case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still confused. Previously, this function, which is called from multiple code paths, did not call setConfigItem . Now it does. This to me seems like a behavior change that is not part of "Always showing the sidebar in case of settings page".

If this is fixing another issue that you noticed while working on this PR, maybe it should be its own commit.

But to me it looks like it is introducing a bug. I think previously, the behavior was we have a setting for whether the sidebar is hidden in general in the configuration that you access via the settings panel, and also there's a quick way to toggle it without changing what happens on next startup. This seems to make clicking the quick toggle always change it on next startup.

I don't really understand the purpose of adding that setConfigItem here at all, but that's my understanding of what impacts it has.


this.$sidebar.classList.toggle("sidebar-hide", !show);
}

Expand Down