[kget] Fix a crash when opening the transfer history dialog
ClosedPublic

Authored by anthonyfieroni on Nov 10 2017, 4:32 AM.

Details

Summary

TransferHistory is derived from Ui::TransferHistory, it can remove all duplicate members.

Test Plan

On first look it works, Wolfgang verify it.

Diff Detail

Repository
R433 KGet
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
anthonyfieroni created this revision.Nov 10 2017, 4:32 AM
anthonyfieroni edited the summary of this revision. (Show Details)Nov 10 2017, 4:39 AM
anthonyfieroni edited the test plan for this revision. (Show Details)
anthonyfieroni edited the summary of this revision. (Show Details)Nov 10 2017, 4:43 AM
wbauer requested changes to this revision.Nov 11 2017, 11:49 AM

Wow, great!
That does indeed fix the crash, and everything seems to work as before.

See below for a mistake I spotted.

There is one problem with this though, the dialog's height is much too small now, and it doesn't remember the size either.
(worked fine before though)

ui/history/transferhistory.cpp
62–63

You swapped them by mistake.
iconView should get "view-list-icons", and listView "view-list-details".

65–66

Same here, iconView and listView are swapped, so the buttons' functionality are reversed.

This revision now requires changes to proceed.Nov 11 2017, 11:49 AM
anthonyfieroni marked 2 inline comments as done.

Remove unused sizeHint.
Make resize in ShowEvent or we should have a temporary widget in TransferHistory, because resize is in base constructor before setupUi call.

Thank you, the size is remembered now and it uses the size stored in kgetrc.

But it still opens too small the first time, if there is no kgetrc...

Maybe sizeHint is for that but it's not a big dial, no? You can always resize at your need.

Maybe sizeHint is for that

I thought so as well, but leaving it in doesn't seem to help either...

It would help to set a default size in the KConfigGroup::readEntry() call, but that would affect other dialogs as well.

but it's not a big dial, no? You can always resize at your need.

Sure, but ideally it shouldn't be necessary. And it's not that it's slightly too small, but you cannot even see a single row of downloads...

I would actually say "ship it", but the other "fix" doesn't have that problem.
And I feel a bit uneasy because I'd like to be sure the initial size can be fixed too (without a bigger rework).
As I said, I definitely like this, but I'd want to spend more time on it first.

Does we push it for 17.12?

wbauer added a comment.Dec 6 2017, 9:26 PM

Does we push it for 17.12?

No from my side.

IMHO, that's a too big change at this point.
And TBH, I haven't even tried your latest change yet.

The crash is fixed by D8730 anyway, so let's better target this for 18.04...

wbauer accepted this revision.Dec 6 2017, 10:40 PM

Actually I did try the latest version a bit now, and it seems to work fine AFAICT.
So feel free to push it, if you are confident about it.

I'm still not sure about 17.12.0 though (maybe better .1?), but it's your decision.

This revision is now accepted and ready to land.Dec 6 2017, 10:40 PM
This revision was automatically updated to reflect the committed changes.