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.

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

Delay initialization of SettingsDialog #2580

Merged
merged 2 commits into from
Oct 22, 2020
Merged

Conversation

misch7
Copy link
Member

@misch7 misch7 commented Oct 22, 2020

As discussed with @er-vin in #2197's review.

  • ownCloudGui::slotShowSettings already got what it takes to create it only when we try to show it for the first time:

    https://github.com/nextcloud/desktop/blob/2b16ab2565ae7c7a8bfdb86f58daa1d26d342e6e/src/gui/owncloudgui.cpp#L556-L564

    This however has some implications:

    Pros:

    • Only created when needed, while testing saved ca. 20 MB of RAM and got freed again after closing the dialog.
    • Since we defaulted to the new Tray UI from 3.0, this is an added bonus for users don't opening the settings.

    Cons:

    • Resources like the avatar image have to be refetched everytime the dialog is recreated.
      This may be desired as well, because it ensures displaying no outdated info (e.g. on connection issues).
  • Fix crash in SettingsDialog with delayed initialization
    setWindowFlags triggered changeEvent, thus causing a crash in customizeStyle.

    This fix should be kept even if we decide against delayed init in the future.

@misch7 misch7 added this to the Desktop 3.1 milestone Oct 22, 2020
@misch7 misch7 requested a review from er-vin October 22, 2020 03:14
@er-vin
Copy link
Member

er-vin commented Oct 22, 2020

/rebase

ownCloudGui::slotShowSettings already got what it takes to create it only when we try to show it for the first time.

This however has some implications:

Pros:
- Only created when needed, while testing saved ca. 20 MB of RAM and got freed again after closing the dialog.
- Since we defaulted to the new Tray UI from 3.0, this is an added bonus for users don't opening the settings.

Cons:
- Resources like the avatar image have to be refetched everytime the dialog is recreated.
  This may be desired as well, because it ensures displaying no outdated info (e.g. on connection issues).

Signed-off-by: Michael Schuster <[email protected]>
setWindowFlags triggered changeEvent, thus causing a crash in customizeStyle.

This fix should be kept even if we decide against delayed init in the future.

Signed-off-by: Michael Schuster <[email protected]>
@github-actions github-actions bot force-pushed the delay-settingsdialog-init branch from 2b16ab2 to 7699004 Compare October 22, 2020 11:29
@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-2580-7699004a11f896962dc7c6ba0b0920fad3dc902b-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@er-vin er-vin merged commit b982a17 into master Oct 22, 2020
@er-vin er-vin deleted the delay-settingsdialog-init branch October 22, 2020 13:08
er-vin pushed a commit that referenced this pull request Nov 9, 2020
Because of PR #2580 the settings dialog doesn't always exist. We need to
check for it first before placing calls to it.

Signed-off-by: Kevin Ottens <[email protected]>
github-actions bot pushed a commit that referenced this pull request Nov 10, 2020
Because of PR #2580 the settings dialog doesn't always exist. We need to
check for it first before placing calls to it.

Signed-off-by: Kevin Ottens <[email protected]>
PraMiD pushed a commit to PraMiD/nextcloud-desktop that referenced this pull request Dec 30, 2020
Because of PR nextcloud#2580 the settings dialog doesn't always exist. We need to
check for it first before placing calls to it.

Signed-off-by: Kevin Ottens <[email protected]>
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