Skip to content

Conversation

mabaasit
Copy link
Collaborator

@mabaasit mabaasit commented Jun 29, 2023

Description

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

});

it('notifies callers of preferences changes after fetchPreferences', async function () {
it.skip('handles concurrent modifications to different preferences', async function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just leaving a comment here about the .skip()

@mabaasit mabaasit changed the title preferences storage chore(storage-mixin): remove from preferences storage COMPASS-6946 Jul 11, 2023
@mabaasit mabaasit requested a review from addaleax July 11, 2023 10:38
@mabaasit mabaasit marked this pull request as ready for review July 11, 2023 10:49
): Promise<AllPreferences> {
const keys = Object.keys(attributes) as (keyof UserPreferences)[];
const originalPreferences = await this.fetchPreferences();
const originalPreferences = this.getPreferences();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to mention it explicitly and make sure it’s intentional, this means that if a user has two Compass instances running and changes preferences first in one and then the other, now that second write always undoes the first one, rather than merging the updates

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh! This seems like a breaking change i am introducing. I think I need to align this and avoid any unwanted behaviour.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While going through the changes, I stumbled on this page and realized that electron has a single main process and all the browser windows share it. So any change made in any Compass window should reflect on other and should not be out of sync. Plus I also tested it and works just fine. Let me know what you think about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Two instances of the app or two windows in the same app?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

multiple windows within the same app.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mabaasit Right, that’s how it works on macOS or when you use the “New Connection” button. On other OSes, starting Compass multiple times through the regular mechanism for doing so will create actual different Compass main processes.

Again, no particularly strong feelings here and this is still a change in behavior that we can make, we should just be intentional about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added 2e12f02 to address this.

However, when we have two different compass instances and user updates any preferences it will not be reflected in other one unless some user action triggers fetching of preferences (which is the current behaviour).

mabaasit and others added 5 commits July 11, 2023 21:07
Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>
Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>
Co-authored-by: Anna Henningsen <anna.henningsen@mongodb.com>
@mabaasit mabaasit merged commit 75ce902 into main Jul 12, 2023
@mabaasit mabaasit deleted the storage-mixin-from-user-preferences branch July 12, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants