Patch for T2047 "Make "Disk Usage window" a regular window (if this takes too long, make it a modal window)"
ClosedPublic

Authored by miroslavm on Jul 2 2017, 8:53 PM.

Details

Summary

The disk usage dialog is modified to be non modal as requested by T2047 and appears in the task bar.
Close button of dialog is connected to close slot of QDialog. closeEvent() was overridden to have same behavior as previously.
Fixed segmentation fault if user cancels the dialog.

NOTE: I'm pretty new with Qt/KDE development. Any comments are welcome!
Test Plan

Open the disk usage dialog. Change the directory and open a second disk usage dialog. Close all disk usage dialogs.
Open the disk usage dialog, press cancel when asked for directory to list.

Diff Detail

Repository
R167 Krusader
Lint
Lint Skipped
Unit
Unit Tests Skipped
miroslavm created this revision.Jul 2 2017, 8:53 PM
martinkostolny accepted this revision.Jul 3 2017, 5:10 AM
martinkostolny added a subscriber: martinkostolny.

Looks good to me and works. Thanks for your code! Please see one code comment.

PS: This diff has no reviewers, you should add Krusader group as reviewers.

krusader/DiskUsage/diskusagegui.cpp
157

This show() here is redundant. I'd remove it.

This revision is now accepted and ready to land.Jul 3 2017, 5:10 AM
abika added a subscriber: abika.Jul 3 2017, 6:48 PM
abika added inline comments.
krusader/krslots.cpp
688

Memory leak here!

martinkostolny added inline comments.Jul 4 2017, 9:25 AM
krusader/krslots.cpp
688

Alex, I'm actually not sure how to resolve this leak. It's the creation of a new window which is destructed by closing it or closing Krusader. Other non-modal windows (Synchronizer, Search, Locate) are created in similar fashion. I'm probably not experienced enough for this, please advise :).

miroslavm added inline comments.Jul 4 2017, 5:49 PM
krusader/krslots.cpp
688

Hello,
I have also thought that the destructor will destroy the object after closing. I have tried to find a hint in the Qt documentation as well as asked a popular search engine.
Would setting the attribute "Qt::WA_DeleteOnClose" solve the issue (see below)?
If so, the other non-modal dialogs have to be changed too.

DiskUsageGUI *du = new DiskUsageGUI(ACTIVE_PANEL->virtualPath(), 0);
du->setAttribute(Qt::WA_DeleteOnClose);
du->show();
abika added a comment.Jul 4 2017, 7:08 PM
setAttribute(Qt::WA_DeleteOnClose)

is a good solution (StackOverflow is your friend:) https://stackoverflow.com/q/20491864/6286694) . You can do this inside the constructor.

And a matter of taste is keeping the parent argument for the constructor (keep MAIN_VIEW instead of 0). This will make the dialog a child the mainwindow but with show() it will not block. If you don't like this (I don't care) the parent parameter should be removed.

For Search and Locate only one dialog instance is created. SynchronizerGUI has indeed the same problem (for the last 7+ years). And krusader/UserAction/expander.cpp:702 looks strange.

This comment was removed by miroslavm.
miroslavm updated this revision to Diff 16222.Jul 5 2017, 9:26 PM
miroslavm added a reviewer: Krusader.

Update after review

In D6471#121701, @abika wrote:
setAttribute(Qt::WA_DeleteOnClose)

is a good solution (StackOverflow is your friend:) https://stackoverflow.com/q/20491864/6286694) . You can do this inside the constructor.

And a matter of taste is keeping the parent argument for the constructor (keep MAIN_VIEW instead of 0). This will make the dialog a child the mainwindow but with show() it will not block. If you don't like this (I don't care) the parent parameter should be removed.

For Search and Locate only one dialog instance is created. SynchronizerGUI has indeed the same problem (for the last 7+ years). And krusader/UserAction/expander.cpp:702 looks strange.

Hello,
should the SynchronizerGUI issue be resolved with T2047 or should a new task be created? I have already prepared a patch. Regarding the expander.cpp I have no clue what to do here.
Regards

Sorry for my ignorance with the QDialog and memory leak issue, I've now learnt from it:). I believe the code now looks fine, and works. Although I'd rather remove the parent parameter (0) from new DiskUsageGUI call or replace with MAIN_VIEW like Alex was advising.

should the SynchronizerGUI issue be resolved with T2047 or should a new task be created? I have already prepared a patch. Regarding the expander.cpp I have no clue what to do here.

In my opinion, if there is not much to discuss before a creation of new patch, there is no need to create a new task for that. But of course it doesn't hurt. I'd rather not connect the Synchronizer patch to task T2047 since the topic was a bit different. As for the expander.cpp:702, I believe it should be fixed the same way as creation of SynchronizerGUI QDialog in krslots. Or is there another problem I cannot see?

abika accepted this revision.EditedJul 6 2017, 6:06 PM

I fixed the synchronizer dialog myself, wasn't that hard. -> 67c400b5

Patch is ok. The double show() and useless 0 argument is still there.
@miroslavm you can merge if you have write access.

miroslavm updated this revision to Diff 16344.Jul 8 2017, 12:56 PM
miroslavm edited the summary of this revision. (Show Details)
miroslavm edited the test plan for this revision. (Show Details)

Update after review. DiskUsage dialog non-modal. Segmentation fault fixed.

In D6471#122384, @abika wrote:

I fixed the synchronizer dialog myself, wasn't that hard. -> 67c400b5

Patch is ok. The double show() and useless 0 argument is still there.
@miroslavm you can merge if you have write access.

Hello,

I had to update the patch due to a segmentation fault when the user cancels the dialog when ask for the directory.
I have used also your code for the maximization.
I have unfortunately forgotten to delete the 0 :-(

I have no write access to the repository at least I don't know.

Sorry for any inconvenience by submitting so many diffs ... I'll try to change this ;-)
Regards and have a nice weekend!

Closed by commit R167:6687fad746d2: Make "Disk Usage window" a regular window (authored by miroslavm, committed by Alexander Bikadorov <alex.bikadorov@kdemail.net>). · Explain WhyJul 11 2017, 8:14 PM
This revision was automatically updated to reflect the committed changes.
abika added a comment.Jul 11 2017, 8:18 PM

Merged. Thanks for the patch!

Sorry for any inconvenience by submitting so many diffs ... I'll try to change this ;-)

Don't worry. That's totally normal and why we have the reviews. My fault I didn't see the segfault.

The code was problematic to begin with. A constructor should construct an object and not do other things like showing a dialog. I took the liberty to make changes afterwards.
Please try to use arc next time for submitting a diff. It makes merging easier.