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

qt: Cleanup SplashScreen class #14854

Merged
merged 1 commit into from Dec 7, 2018

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Dec 1, 2018

Cleaning up after replacing the QSplashScreen base class with the QWidget class (#4941 by @laanwj).

cc @jonasschnelli

Cleaning up after replacing the QSplashScreen base class with the
QWidget class.
@fanquake fanquake added the GUI label Dec 2, 2018
splash->show();
connect(this, &BitcoinApplication::splashFinished, splash, &SplashScreen::slotFinish);
connect(this, &BitcoinApplication::splashFinished, splash, &SplashScreen::finish);
Copy link
Member

@promag promag Dec 3, 2018

Could just

connect(this, &BitcoinApplication::splashFinished, splash, &SplashScreen::deleteLater);

or is there a good reason to call hide()?

Copy link
Member

@promag promag Dec 3, 2018

When #8231 was merged there was no deleteLater() call.

Copy link
Member Author

@hebasto hebasto Dec 3, 2018

Tested on macOS 10.13.6: if the splash screen is minimized, neither deleteLater() nor hide(); deleteLater() can remove it from the Dock.
It seems the simplest way is to not touch the patch from #8231.

Copy link
Member Author

@hebasto hebasto Dec 3, 2018

@promag Moreover, IIUC, 34ebceb commit has been effectively disabled by e4f126a.
What do you think about this?

Copy link
Member

@promag promag Dec 3, 2018

Just tested my suggestion above with Qt 5.11 on macOS 10.14.1 and the following are good:

  • closing the app while splash is open
  • splash goes away from the dock even if it's minimized.

What's your Qt version?

Copy link
Member Author

@hebasto hebasto Dec 4, 2018

@promag

What's your Qt version?

Qt 5.11.2

Copy link
Member

@promag promag Dec 4, 2018

@hebasto with my above suggestion I can't reproduce #7718 in my system. Maybe this is was macOS issue?

Copy link
Member Author

@hebasto hebasto Dec 5, 2018

@promag

Maybe this is was macOS issue?

Agree. As there are systems subject to #7718 it is better to not touch the patch from #8231, IMO.

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Dec 3, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14250 (qt: Remove redundant stopThread() and stopExecutor() signals by hebasto)
  • #13880 (gui: Try shutdown also on failure by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Dec 7, 2018

Can't remember why the unused variable ended up there.

utACK 7d1b60c

@laanwj
Copy link
Member

@laanwj laanwj commented Dec 7, 2018

utACK 7d1b60c

pull bot pushed a commit to ken2812221/bitcoin that referenced this issue Dec 7, 2018
7d1b60c Cleanup SplashScreen class (Hennadii Stepanov)

Pull request description:

  Cleaning up after replacing the `QSplashScreen` base class with the `QWidget` class (bitcoin#4941 by @laanwj).

  cc @jonasschnelli

Tree-SHA512: 72e2d67905d85247a11ae6a884f74f710f765adf20db7d1daf0927e6990687e836b486c4ff93bc6dabc3759ed667acfe1d69c8b94fae7181ab271a3fa7a0229a
@laanwj laanwj merged commit 7d1b60c into bitcoin:master Dec 7, 2018
2 checks passed
@hebasto hebasto deleted the 20181201-cleanup-splashscreen branch Dec 7, 2018
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Oct 9, 2020
Summary:
Cleaning up after replacing the QSplashScreen base class with the
QWidget class ([[bitcoin/bitcoin#4941 | PR4941]]).

Backport of Core [[bitcoin/bitcoin#14854 | PR14854]]

Test Plan:
```
ninja && ninja check
src/qt/bitcoin-qt
```

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7840
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants