Remove window size/position restoration on help and about windows #5772

ghost commented at 9:12 PM on February 8, 2015: none

Removes the unecessary function to restore the about window size and location for the about window when reopening the window.

Currently the window size and location is restored even after application restart.

jonasschnelli commented at 9:16 PM on February 8, 2015: contributor

I didn't test, but the debug / rpc console has somehow the same code basis. I assume this PR will no longer store the position of the debug window as well? Maybe you should take a look at HelpMessageDialog::HelpMessageDialog about boolean.

jonasschnelli commented at 9:17 PM on February 8, 2015: contributor

conceptional ACK on not storing the about window geometries.

ghost commented at 9:30 PM on February 8, 2015: none

@jonasschnelli I believe that does include the debug window as it currently is. If it's a big deal to remove the functionality for the debug window, would there be any issue with splitting help message (i.e. the debug window) and about messages in to two classes? I see only six references (not including translation files) to HelpMessageDialog.

unknown renamed this:
~~Removes window size/position restoration on reopen~~
Removes window size/position restoration on window and app reopen
on Feb 8, 2015

Diapolo commented at 6:50 AM on February 9, 2015: none

Why? And also it's not sufficient to just remove the restoration code but also the save code...

in src/qt/utilitydialog.cpp:None in a8560d60ad outdated

  27 | @@ -28,9 +28,6 @@ HelpMessageDialog::HelpMessageDialog(QWidget *parent, bool about) :
  28 |      QDialog(parent),
  29 |      ui(new Ui::HelpMessageDialog)
  30 |  {
  31 | -    ui->setupUi(this);

jonasschnelli commented at 7:45 AM on February 9, 2015:

Why did you remove this line? This will not work.

in src/qt/utilitydialog.cpp:None in a8560d60ad outdated

  27 | @@ -28,9 +28,6 @@ HelpMessageDialog::HelpMessageDialog(QWidget *parent, bool about) :
  28 |      QDialog(parent),
  29 |      ui(new Ui::HelpMessageDialog)
  30 |  {
  31 | -    ui->setupUi(this);
  32 | -    GUIUtil::restoreWindowGeometry("nHelpMessageDialogWindow", this->size(), this);

jonasschnelli commented at 7:46 AM on February 9, 2015:

This needs a if(!about) and not a remove.

laanwj commented at 9:18 AM on February 9, 2015:

I'm not sure it is terribly useful for the help message dialog to store it's position either.

jonasschnelli commented at 9:20 AM on February 9, 2015:

Indeed.

jonasschnelli commented at 7:47 AM on February 9, 2015: contributor

At the destructor HelpMessageDialog::~HelpMessageDialog() you must also make sure, geometry gets stored only if about==false. You can add a fAbout as private instance variable.

Michagogo commented at 7:58 AM on February 9, 2015: contributor

NAK on discarding debug window size/location. Indifferent regarding the about window (will this also affect the command line options window?), but the debug window is one which can actually be useful and which a user might reasonably want to have in a fixed location and not have to reposition it each time.

jonasschnelli commented at 8:04 AM on February 9, 2015: contributor

@Michagogo no no. The Help- (command line) and Debug-window will keep storing the position. This commit (https://github.com/jonasschnelli/bitcoin/commit/a16b79b16d9f13f13f13960e858d9416694a2ee4) would only affect the about window. No others. I try to guide @L-Cranston-Shadow into this direction.

Not storing other windows geometry would be stupid. But the about window really don't need stored window geometries. The about window looks better without storing geometries and more like the "About Qt" window.

Michagogo commented at 8:08 AM on February 9, 2015: contributor

@jonasschnelli That commit sounds good, yeah. That's not what this PR is right now, though.

laanwj added the label GUI on Feb 9, 2015

laanwj commented at 9:19 AM on February 9, 2015: member

Concept ACK - but please adapt your commit message to make it clear that only the HelpMessage/About dialog is affected and not every window. That kind of confused me and I see I'm not the only one.

unknown renamed this:
~~Removes window size/position restoration on window and app reopen~~
Removes window size/position restoration on help and about window and app reopen
on Feb 9, 2015

unknown renamed this:
~~Removes window size/position restoration on help and about window and app reopen~~
Remove window size/position restoration on help and about windows
on Feb 9, 2015

ghost commented at 11:05 AM on February 9, 2015: none

Sorry for the delay in replying, @Diapolo It was unclear in my original title but this PR is targeted specifically at the about window. The reasoning behind it was that the functionality of restoring the window size and position when reopening is unecessary so I, foolishly in retrospect, thought it would be a good way to dip my feet into both contributing to the project and getting back into C++. @jonasschnelli Removing that extra line was a mistake, I'll make sure to re-add it @laanwj Thanks, I renamed the PR and I'll use a better summary for the next commit

Everyone, I'll try to get a new commit up soon on this, I'm a little bit out of my depth both in terms of the specific code and the programming, some of which is stuff that I learned and I am having to pretty much relearn/remember or never learned and am having to teach myself on, so please bear with me.

laanwj commented at 11:45 AM on February 9, 2015: member

Oops. Something went wrong here, github reports a lot of added commits which were already merged.

Try to use rebase (e.g. git pull --rebase) instead of merge when you want to forward-port a pull request, so we can maintain a linear-ish history, as parallel merge commits make it hard to review.

unknown closed this on Feb 9, 2015

ghost commented at 12:13 PM on February 9, 2015: none

See the above link, I somehow screwed something up and in the aftermath the system automatically closed this PR. Rather than email GitHub and ask for them to look into it, I figured it would be more expedient and almost as easy to create a new one.

MarcoFalke locked this on Sep 8, 2021