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.
Details
- Reviewers
martinkostolny abika - Group Reviewers
Krusader - Commits
- R167:6687fad746d2: Make "Disk Usage window" a regular window
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
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
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. |
krusader/krslots.cpp | ||
---|---|---|
688 | Memory leak here! |
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 :). |
krusader/krslots.cpp | ||
---|---|---|
688 | Hello, DiskUsageGUI *du = new DiskUsageGUI(ACTIVE_PANEL->virtualPath(), 0); du->setAttribute(Qt::WA_DeleteOnClose); du->show(); |
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?
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!
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.