Ignore Synchronize Folders redundant resize

Authored by mporterfield on May 26 2019, 6:37 PM.


Group Reviewers

When the Synchronize Folders tool is opened maximized, it receives two
resize events instead of one.

(-1,-1) -> (W,H)
(100,30) -> (W,H)

This results in the Name column growing wider each time the tool is
opened. This change is to ignore a resize event when the window is
already the new size.

BUG: 167410

Diff Detail

Lint Skipped
Unit Tests Skipped
mporterfield requested review of this revision.May 26 2019, 6:37 PM
mporterfield created this revision.
nmel added a subscriber: nmel.May 27 2019, 6:50 AM

Thank you for the patch, Mark!

From the code and logic side it looks good. Needs some testing by people actively using the Synchronizer (I don't).


Logically, it's fine, however I think it will be more clear if it is in a form of

if (maximized) {
  // maximize the window
  setWindowState(windowState() | Qt::WindowMaximized);
} else {
  // resize if the window size is stored in the config
  if (sx != -1 && sy != -1)
    resize(sx, sy);

Updated based on review.

abika added a subscriber: abika.EditedJun 30 2019, 5:27 PM

Sorry for the very long delay.
I tested this but unfortunately there is still the same faulty behaviour.
The first resize event for me has the size
(1019, 651)
the second one the correct (1920, 1019). But e->oldSize().width() is returning 100 here so the computation is still wrong.

This all looks pretty messy including the computation formula for adjusting the column width.
So instead of figuring out what is going on here i propose D22174 . This will make the two "Name" columns stretch to the available space and all other columns with adjust to the width of the content (because this is kind of fixed). The downside is that the column widths cannot be changed by the user anymore (changing the column order still works). What do you think?

If oldSize is still incorrect, how about ignoring it completely in favor of the saved sizeX/sizeY?

I'm also fine with removing the ability to resize columns as in D22174, having never needed to do that except to correct this ever-increasing Name column width.

abika added a comment.Jul 21 2019, 4:01 PM

@mporterfield I merged D22174, please close this review.
BTW: I didn't want to "steal" your fix. I just think it's better to have a solution making the code cleaner for the future.

mporterfield abandoned this revision.Jul 22 2019, 3:47 AM