Review Request 118994: make CTRL+SHIFT+T reopen last closed tab

[Emmanuel Pescosta]

> On July 5, 2014, 1:36 p.m., Arjun Ak wrote:
> > dolphin/src/dolphinrecenttabsmenu.cpp, line 102
> > <https://git.reviewboard.kde.org/r/118994/diff/5/?file=287351#file287351line102>
> >
> >     Is it safe to 'delete' a QObject? shouldnt we be using deleteLater()
> 
> Thomas Lübking wrote:
>     There's no general rule.
>     Deleting a QObject is as un/safe as deleting anything else.
>     
>     You must ensure that the referenced memory is not used afterwards, what makes deleting "stuff" in eventfilters and slots "risky" unless you suck the event (return true) or queue the slot/know that the emitting code is safe against lossing "this" object (ie. it's a "special" problem if you connect to an objects signal and delete that very object from the slot if the emitting code does not return after the emit but tries to access members)
>     This does however hold for every other non-local heap structure (ie. if you delete away an objects member w/o the object noticing it, you cause a dangeling member pointer)
>     
>     ::deleteLater() bypasses *some* problems, but is no bullet proof deletion.
>     Eg. the code around the patch sets the local action pointer "0", what is completely pointless (and makes me worry somebody did sth. w/o any idea /what/ he's doing ;-)
>     
>     
>     If you need protection against randomly deleted objects, you've to use a "smart" pointer (QPointer) and then test whether the pointer ::isNull() if you're unsure about the objects state.

Deleting the triggered action which is passed on by QMenu::triggered(QAction*) is safe.  
(There is also an unit test to ensure that it doesn't cause any crashes - see https://qt.gitorious.org/qt/qtbase/source/ff31090d07cbbb6f67d259438939e810a0baf67f:tests/auto/widgets/widgets/qmenu/tst_qmenu.cpp#L112-836 for more details)

> local action pointer "0", what is completely pointless
Yes it's pointless to set local pointers to 0 on immediate-return, but in this case it makes sense (non immediate-return) to set invalid pointers to 0 to minimize programming errors, because when someone extends this method in future and he wants to use the (possibly invalid) action pointer, it can cause problems when he doesn't notice that the pointer can be invalid. (and when you take a look at our code, we do this in multiple places where we delete local  pointers in non immediate-return code paths).


- Emmanuel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118994/#review61642
-----------------------------------------------------------


On July 6, 2014, 12:46 p.m., Arjun Ak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118994/
> -----------------------------------------------------------
> 
> (Updated July 6, 2014, 12:46 p.m.)
> 
> 
> Review request for Dolphin and KDE Base Apps.
> 
> 
> Bugs: 336818
>     http://bugs.kde.org/show_bug.cgi?id=336818
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Upon pressing CTRL+SHIFT+T, the most recently closed tab is reopened (with the help of Go->'Recently closed tabs' menu).
> 
> 
> Diffs
> -----
> 
>   dolphin/src/dolphinmainwindow.h 7c3bff4 
>   dolphin/src/dolphinmainwindow.cpp d9fe645 
>   dolphin/src/dolphinrecenttabsmenu.h 34d4153 
>   dolphin/src/dolphinrecenttabsmenu.cpp a39f994 
>   dolphin/src/dolphinui.rc 52826bb 
> 
> Diff: https://git.reviewboard.kde.org/r/118994/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Arjun Ak
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140706/c0b80af3/attachment.htm>