Skip to content
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

Jump to bottom

Fix/about page desktop window #8461

Merged
merged 7 commits into from
Oct 25, 2024
Merged

Fix/about page desktop window #8461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2bcac1f
fix: about page desktop window
syns2191 Oct 14, 2024
4320a2a
Merge branch 'develop' of https://github.com/ever-co/ever-gauzy into …
syns2191 Oct 18, 2024
641ab37
fix: about window not fit in windows platform
syns2191 Oct 18, 2024
f8cfb51
fix: about window not fit in windows platform
syns2191 Oct 18, 2024
7072c60
fix: prevent multiple about window
syns2191 Oct 18, 2024
e5517ff
fix: prevent timer window change size on drag the window
syns2191 Oct 18, 2024
50d3883
fix: remove unnecessary code
syns2191 Oct 19, 2024
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
4 changes: 4 additions & 0 deletions apps/desktop-timer/src/preload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ window.addEventListener('DOMContentLoaded', async () => {
titleBar.refreshMenu();
});

ipcRenderer.on('hide-menu', () => {
titleBar.dispose();
})

const overStyle = document.createElement('style');
overStyle.innerHTML = `
.cet-container {
Expand Down
4 changes: 4 additions & 0 deletions apps/desktop/src/preload/preload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ window.addEventListener('DOMContentLoaded', async () => {
titleBar.refreshMenu();
});

ipcRenderer.on('hide-menu', () => {
titleBar.dispose();
})

ipcRenderer.on('adjust_view', () => {
clearInterval(contentInterval);
const headerIcon = '/html/body/div[2]/ga-app/ngx-pages/ngx-one-column-layout/nb-layout/div[1]/div/div/nb-sidebar[1]/div/div/div';
Expand Down
4 changes: 4 additions & 0 deletions apps/server-api/src/preload/preload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ window.addEventListener('DOMContentLoaded', async () => {
titleBar.refreshMenu();
});

ipcRenderer.on('hide-menu', () => {
titleBar.dispose();
})
Comment on lines +20 to +22
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Codebase verification

Potential Issue Identified with titleBar.dispose() Implementation

The introduced 'hide-menu' event listener disposes of the titleBar, but there is no corresponding mechanism to recreate or show the menu again. This can lead to scenarios where the menu remains hidden indefinitely without a way to restore it.

Affected Files:

  • apps/server-api/src/preload/preload.ts
  • apps/server/src/preload/preload.ts
  • apps/desktop-timer/src/preload.ts
  • apps/desktop/src/preload/preload.ts
🔗 Analysis chain

Verify the implications of disposing the titleBar

The addition of the 'hide-menu' event listener enhances the application's ability to dynamically hide the menu, which can be beneficial for certain UI states or user preferences. However, there are a few concerns to address:

  1. There's no visible mechanism to recreate the titleBar after it's disposed. This could lead to issues if the menu needs to be shown again.
  2. The titleBar variable is not declared with a block scope (let or const), which could lead to unexpected behavior if it's reassigned elsewhere in the code.

To ensure this change doesn't introduce unintended side effects, please run the following checks:

Consider adding a mechanism to recreate the titleBar if it needs to be shown again after being hidden. Also, ensure that disposing of the titleBar doesn't cause any side effects in other parts of the application that might depend on its existence.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for a corresponding 'show-menu' event or similar mechanism
rg --type typescript "ipcRenderer.on\('show-menu'" apps/

# Check for other uses of titleBar that might be affected by its disposal
rg --type typescript "titleBar\." apps/

# Verify that no other part of the app depends on the titleBar's existence after it might be disposed
rg --type typescript "CustomTitlebar" apps/

Length of output: 255

Script:

#!/bin/bash
# Check for a corresponding 'show-menu' event or similar mechanism
rg --type ts "ipcRenderer.on\('show-menu'" apps/

# Check for other uses of titleBar that might be affected by its disposal
rg --type ts "titleBar\." apps/

# Verify that no other part of the app depends on the titleBar's existence after it might be disposed
rg --type ts "CustomTitlebar" apps/

Length of output: 1406

Copy link
Member

Choose a reason for hiding this comment

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

@syns2191 I also wonder, what happens after you dispose it!? You seems not recreating it again? What is logic in that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's only disposed when about window is created. so it does not affect to another window

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hide-menu listener only called when creating the About window. I think we don't show menu inside about window

Copy link
Member

Choose a reason for hiding this comment

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

@syns2191 I totally don't get that... I would prefer to NOT dispose anything that later might be needed etc. Hide - yes, dispose - hm... @adkif can you review this and let us know what you think!?

Copy link
Contributor

Choose a reason for hiding this comment

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

@evereq, @syns2191 is right, it's just an "About Window", we don't need to attach a title bar, menubar, etc... to it. That is why the dispose() method invocation.


const overStyle = document.createElement('style');
overStyle.innerHTML = `
.cet-menubar-menu-container {
Expand Down
4 changes: 4 additions & 0 deletions apps/server/src/preload/preload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ window.addEventListener('DOMContentLoaded', async () => {
titleBar.refreshMenu();
});

ipcRenderer.on('hide-menu', () => {
titleBar.dispose();
})

const overStyle = document.createElement('style');
overStyle.innerHTML = `
.cet-menubar-menu-container {
Expand Down
2 changes: 1 addition & 1 deletion packages/desktop-libs/src/lib/desktop-menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export class AppMenu {
label: TranslateService.instant('MENU.ABOUT'),
enabled: true,
async click() {
const window: BrowserWindow = await createAboutWindow(windowPath.timeTrackerUi);
const window: BrowserWindow = await createAboutWindow(windowPath.timeTrackerUi, windowPath.preloadPath);
window.show();
}
},
Expand Down
10 changes: 6 additions & 4 deletions packages/desktop-ui-lib/src/lib/dialogs/about/about.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
<span>
Copyright © 2020-{{'FOOTER.PRESENT' | translate}}
<span class="link" (click)="openLink('EVER')">{{ application?.companyName}}</span>
<br/>
<br />
{{'FOOTER.RIGHTS_RESERVED' | translate}}
<br/>
<span class="link" (click)="openLink('TOS')">{{'FOOTER.TERMS_OF_SERVICE' | translate}}</span> |
<span (click)="openLink('PRIVACY')" class="link">{{'FOOTER.PRIVACY_POLICY' | translate}}</span>
<br />
<span class="link" (click)="openLink('TOS')">{{'FOOTER.TERMS_OF_SERVICE' |
translate}}</span> |
<span (click)="openLink('PRIVACY')" class="link">{{'FOOTER.PRIVACY_POLICY' |
translate}}</span>
</span>
</div>
</div>
Expand Down
2 changes: 1 addition & 1 deletion packages/desktop-ui-lib/src/lib/dialogs/about/about.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,4 @@ div.logo {

::ng-deep nb-layout .layout .layout-container .content nb-layout-footer nav {
padding: 0px;
}
}
15 changes: 8 additions & 7 deletions packages/desktop-window/src/lib/desktop-window-about.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import * as remoteMain from '@electron/remote/main';
import { BrowserWindow, Menu } from 'electron';
import * as url from 'url';
import { attachTitlebarToWindow } from 'custom-electron-titlebar/main';

import log from 'electron-log';
import { WindowManager } from './concretes/window.manager';
Expand All @@ -16,6 +15,13 @@ export async function createAboutWindow(filePath, preloadPath?) {
const mainWindowSettings: Electron.BrowserWindowConstructorOptions = windowSetting(preloadPath);
const manager = WindowManager.getInstance();

const allwindows = BrowserWindow.getAllWindows();
const aboutWindows = allwindows.find((win) => win.getTitle() === 'About');
if (aboutWindows) {
aboutWindows.show();
return aboutWindows;
}

const window = new BrowserWindow(mainWindowSettings);
remoteMain.enable(window.webContents);
const launchPath = url.format({
Expand All @@ -42,7 +48,7 @@ export async function createAboutWindow(filePath, preloadPath?) {

manager.register(RegisteredWindow.ABOUT, window);
if (preloadPath) {
attachTitlebarToWindow(window);
window.webContents.send('hide-menu');
}
return window;
}
Expand All @@ -69,11 +75,6 @@ const windowSetting = (preloadPath) => {
};
if (preloadPath) {
mainWindowSettings.webPreferences.preload = preloadPath;
mainWindowSettings.titleBarOverlay = true;
mainWindowSettings.titleBarStyle = 'hidden';
if (process.platform === 'linux') {
mainWindowSettings.frame = false;
}
}
return mainWindowSettings;
};
19 changes: 14 additions & 5 deletions packages/desktop-window/src/lib/desktop-window-timer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ Object.assign(console, log.functions);
const Store = require('electron-store');
const store = new Store();

function getScreenSize() {
const sizes = screen.getPrimaryDisplay().workAreaSize;
const width = sizes.height < 768 ? 310 : 360;
const height = sizes.height < 768 ? sizes.height - 20 : 768;
return { width, height }
}

export async function createTimeTrackerWindow(timeTrackerWindow, filePath, preloadPath?) {
const mainWindowSettings: Electron.BrowserWindowConstructorOptions = windowSetting(preloadPath);
const manager = WindowManager.getInstance();
Expand All @@ -29,24 +36,26 @@ export async function createTimeTrackerWindow(timeTrackerWindow, filePath, prelo

timeTrackerWindow.hide();
await timeTrackerWindow.loadURL(launchPath);
if (preloadPath) {
attachTitlebarToWindow(timeTrackerWindow);
}
const { width, height } = getScreenSize();
timeTrackerWindow.setMinimumSize(width, height);
timeTrackerWindow.setMenu(null);
timeTrackerWindow.on('close', (event) => {
event.preventDefault();
timeTrackerWindow.hide();
});

manager.register(RegisteredWindow.TIMER, timeTrackerWindow);
if (preloadPath) {
attachTitlebarToWindow(timeTrackerWindow);
}

return timeTrackerWindow;
}

const windowSetting = (preloadPath?) => {
const sizes = screen.getPrimaryDisplay().workAreaSize;
const height = sizes.height < 768 ? sizes.height - 20 : 768;
const { width, height } = getScreenSize();
const zoomF = sizes.height < 768 ? 0.8 : 1.0;
const width = sizes.height < 768 ? 310 : 360;
const filesPath = store.get('filePath');
console.log('file path == ', filesPath);
const mainWindowSettings: Electron.BrowserWindowConstructorOptions = {
Expand Down
Loading