Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.49k stars 2.85k forks source link

[$250] Desktop window does not keep the desired size #50509

Open m-natarajan opened 2 weeks ago

m-natarajan commented 2 weeks ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.46-4 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @flodnv Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1728478035371609

Action Performed:

  1. Open Desktop App
  2. Resize Desktop App window
  3. Quit Desktop App with Cmd+Q
  4. Open Desktop App

    Expected Result:

    Size of window is the same as step 3 (after resizing it)

    Actual Result:

    Size of window is the same as step 1 -- it resets to some default. We should keep the user's resize.

    Workaround:

    Unknown

    Platforms:

    Which of our officially supported platforms is this issue occurring on?

    • [ ] Android: Native
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Native
    • [ ] iOS: mWeb Safari
    • [ ] MacOS: Chrome / Safari
    • [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/12328452-213c-470b-b70d-3c3d9991ae15

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021844122075299762610
  • Upwork Job ID: 1844122075299762610
  • Last Price Increase: 2024-10-09
Issue OwnerCurrent Issue Owner: @mananjadhav
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

shahinyan11 commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2024-10-10 13:29:44 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Desktop window does not keep the desired size

What is the root cause of that problem?

Here we always set the window size to 1200 x 900 when opening a desktop application.

What changes do you think we should make in order to solve the problem?

We need to save the window dimensions when the application is closed and get them when the application is opened. To do that we can done bellow steps

  1. install 8.2.0 version of electron-store module
  2. Add following lines in desktop/main.ts file
    
    import Store from 'electron-store';

...

const store = new Store()

3. Update [this](https://github.com/Expensify/App/blob/00b7426d3539cc16f0a714885da7cbf0796a04c7/desktop/main.ts#L290C1-L300C20)  code to below

const windowSize = store.get('windowSize');

browserWindow = new BrowserWindow({ backgroundColor: '#FAFAFA', width: windowSize?.width ?? 1200, height: windowSize?.height ?? 900, webPreferences: { preload: ${__dirname}/contextBridge.js, contextIsolation: true, sandbox: false, }, titleBarStyle: 'hidden', });

4. Update this [if](https://github.com/Expensify/App/blob/00b7426d3539cc16f0a714885da7cbf0796a04c7/desktop/main.ts#L524C1-L526C22) block to below

if (quitting || hasUpdate) { const { width, height } = browserWindow.getBounds(); store.set('windowSize', { width, height }); return; }



https://github.com/user-attachments/assets/30d76e7b-e72b-48f0-86ab-0eb4c390ccd7

**Addition** We can also save the window position and open a new window in the previous position.

### What alternative solutions did you explore? (Optional)
melvin-bot[bot] commented 2 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~021844122075299762610

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

huult commented 2 weeks ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Desktop window does not keep the desired size

What is the root cause of that problem?

We have fixed the width and height of the BrowserWindow when initializing the desktop version. This ensures consistency, but it may limit responsiveness and scalability across different screen sizes.

https://github.com/Expensify/App/blob/00b7426d3539cc16f0a714885da7cbf0796a04c7/desktop/main.ts#L292-L293

What changes do you think we should make in order to solve the problem?

We will store the width and height when the desktop app is closed and retrieve them when it is reopened. We should prioritize simplicity, lightweight design, and a focused approach by using straightforward APIs like electron-settings to handle basic configurations.

npm install electron-settings
import settings from "electron-settings";
// ./desktop/main.ts#L290
+      let windowState = settings.getSync('windowState') || {
+                    width: 1200,
+                    height: 900,
+                };

 browserWindow = new BrowserWindow({
-                    width: 1200,
+                   width: windowState.width,
-                    height: 900,
+                   height: windowState.height,
                    ...

// ./desktop/main.ts#L254
if (quitting || hasUpdate) {
// If the app is quitting or an update is available, save the current window size and position to settings.
+                    let {width, height, x, y} = browserWindow.getBounds();
+                    settings.setSync('windowState', {width, height, x, y});
return;
Shahidullah-Muffakir commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

When a user resizes the desktop app window and quits with Cmd+Q, the app does not remember the new size. It resets to the default size when reopened.

What is the root cause of that problem?

The app doesn’t store the window size, so it defaults to the original dimensions when restarted. https://github.com/Expensify/App/blob/00b7426d3539cc16f0a714885da7cbf0796a04c7/desktop/main.ts#L290

What changes do you think we should make in order to solve the problem?

Since we are already using Onyx in our app, we don't need to install any additional packages we will create a new Onyx key windowDimensions in the ONYXKEYS.ts file, to store the window's width and height.

  1. Save the window size in windowDimensions whenever user is closing the app here

              const {width, height} = browserWindow.getBounds();
              Onyx.set('ONYXKEYS.WINDOW_DIMENSIONS', {width, height});
  2. When the app starts, retrieve and apply the saved size.
              let width;
              let height;
              Onyx.connect({
                  key: ONYXKEYS.WINDOW_DIMENSIONS,
                  callback: (windowDimensions) => {
                      width = windowDimensions?.width ?? 1200;
                      height = windowDimensions?.height ?? 900;
                  },
              });
  3. If no size is saved, use the default size.
mananjadhav commented 1 week ago

Will review the proposals in a while.

mananjadhav commented 1 week ago

Overall all the proposals identified the correct root cause. But I feel we should leverage Onyx instead of adding new packages. Hence, I think we should go with this proposal. We still need to workout the proper code logic.

🎀 👀 🎀 C+ reviewed.

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @tylerkaraszewski, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

shahinyan11 commented 1 week ago

@mananjadhav @tylerkaraszewski Hi there. I am sorry but All proposals are very close to mine. I first found the problem and suggested a way to fix the problem with the opinion of fixing the problems. And I do not think that the place where should be saved data significant difference in this case. And in my opinion, the other proposal does not correspond to below point from CONTRIBUTING.md

Note: Before submitting a proposal on an issue, be sure to read any other existing proposals. ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS. The difference should be important, meaningful or considerable.

Once again, I apologize if I'm wrong.

mananjadhav commented 1 week ago

Thanks for raising this @shahinyan11. As I said I can see all proposals did identify the correct root cause. But the implementation details does differ. The change is anyway very trivial. I still stand by with my recommendation.

But I'll let @tylerkaraszewski decide it.

melvin-bot[bot] commented 1 week ago

📣 @Shahidullah-Muffakir You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

Shahidullah-Muffakir commented 1 week ago

Thank you, will submit the PR by tomorrow.

mananjadhav commented 1 week ago

To resolve this issue, we needed to persist the window size between sessions. Several options were considered, including external packages, onyx, local storage, and using the fs module. After exploring these options, I found that using the fs module was the simplest and most straightforward solution, requiring minimal changes to the existing code. This approach allows the window size to be saved to a local JSON file and restored on app reopen.

Starting the conversation here for using the code different from proposal (link). @Shahidullah-Muffakir I don't think we should be using fs. This isn't an ideal solution.

Also, I think if you were to change the approach we should post it on the issue to clarify before implementing the PR. The only reason the proposal was accepted because it reused existing lib.

huult commented 1 week ago

Perhaps we can't use Onyx.connect in index.desktop.ts. I tried it before, but it didn't work, so I tried using the Electron package instead

shahinyan11 commented 1 week ago

Also, I think if you were to change the approach we should post it on the issue to clarify before implementing the PR. The only reason the proposal was accepted because it reused existing lib.

@mananjadhav Of course, that wouldn't be fair.

Shahidullah-Muffakir commented 1 week ago

@mananjadhav, I tried using Onyx, but it was quite difficult to make it work, and even if we could, it would add a lot of unnecessary complexity.

As for fs, it’s not an external package—it’s already built into our environment, we are not installing anything, which is why I chose it. Other plugins would also end up using disk storage, just like fs. For such a small change, I don’t think adding an external package makes sense, since we can get the same result with fs.

Let me know what you think. Thanks!

Shahidullah-Muffakir commented 6 days ago

@mananjadhav,

The main issue I’m having is that I can't access the Onyx data in the main process when creating the main window. The window size is saved correctly when the user quits the app with Cmd+Q, but when the app restarts, I can't connect to Onyx.

I think it might be because Onyx uses browser storage, so before the browser window is created, its storage isn't available (but I’m not sure).

Could you help me figure out how to connect to Onyx before the main window is created? Sorry for the trouble.

shahinyan11 commented 1 day ago

@tylerkaraszewski @mananjadhav Hi guys. Is there any updates, what's next ?