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

[Arjun Ak]

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

(Updated July 22, 2014, 6:03 p.m.)


Status
--

This change has been marked as submitted.


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

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

[Frank Reininghaus]

> On July 20, 2014, 9:56 p.m., Frank Reininghaus wrote:
> > Thank you very much for working on this, Arjun, and thanks also for your 
> > very good suggestions, Emmanuel!
> > 
> > I apologize for not responding earlier. I was a bit busy at work recently 
> > and did not really find the time to check all mails and review request 
> > notifications in my inbox and see which of them could (and should) be 
> > handled quickly nevertheless (like this one).
> > 
> > Looks very good from my point of view. Please go ahead and push it. I have 
> > one minor suggestion for a change, but feel free to ignore it if you 
> > prefer, it's probably a matter of taste.
> > 
> > Thanks again for adding a useful feature, which many users will appreciate.
> 
> Arjun Ak wrote:
> Which branch should this be pushed to master or frameworks?

Push to master, please.


- Frank


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


On July 6, 2014, 1:58 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, 1:58 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
> 
>

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

[Arjun Ak]

> On July 21, 2014, 3:26 a.m., Frank Reininghaus wrote:
> > Thank you very much for working on this, Arjun, and thanks also for your 
> > very good suggestions, Emmanuel!
> > 
> > I apologize for not responding earlier. I was a bit busy at work recently 
> > and did not really find the time to check all mails and review request 
> > notifications in my inbox and see which of them could (and should) be 
> > handled quickly nevertheless (like this one).
> > 
> > Looks very good from my point of view. Please go ahead and push it. I have 
> > one minor suggestion for a change, but feel free to ignore it if you 
> > prefer, it's probably a matter of taste.
> > 
> > Thanks again for adding a useful feature, which many users will appreciate.

Which branch should this be pushed to master or frameworks?


- Arjun


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


On July 6, 2014, 7:28 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, 7:28 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
> 
>

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

[Frank Reininghaus]

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

Ship it!


Thank you very much for working on this, Arjun, and thanks also for your very 
good suggestions, Emmanuel!

I apologize for not responding earlier. I was a bit busy at work recently and 
did not really find the time to check all mails and review request 
notifications in my inbox and see which of them could (and should) be handled 
quickly nevertheless (like this one).

Looks very good from my point of view. Please go ahead and push it. I have one 
minor suggestion for a change, but feel free to ignore it if you prefer, it's 
probably a matter of taste.

Thanks again for adding a useful feature, which many users will appreciate.


dolphin/src/dolphinmainwindow.cpp


I would prefer setEnabled(count > 0) (because it makes it clear at first 
sight that the argument is a bool), but that might be a matter of taste.


- Frank Reininghaus


On July 6, 2014, 1:58 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, 1:58 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
> 
>

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

[Emmanuel Pescosta]

> On July 8, 2014, 7:56 p.m., Emmanuel Pescosta wrote:
> > Thanks for your work and sorry for the delay! I like it :)
> > 
> > A ship it from my side. (I still prefer "count > 0" over "count", but 
> > that's more a matter of taste)
> > 
> > Please wait for Frank's ship it.

@Frank: ping!


- Emmanuel


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


On July 6, 2014, 3:58 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, 3:58 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
> 
>

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

[Emmanuel Pescosta]

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

Ship it!


Thanks for your work and sorry for the delay! I like it :)

A ship it from my side. (I still prefer "count > 0" over "count", but that's 
more a matter of taste)

Please wait for Frank's ship it.

- Emmanuel Pescosta


On July 6, 2014, 3:58 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, 3:58 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
> 
>

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

[Arjun Ak]

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

(Updated July 6, 2014, 7:28 p.m.)


Review request for Dolphin and KDE Base Apps.


Changes
---

Fixed mentioned issues


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 (updated)
-

  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

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

[Emmanuel Pescosta]

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



dolphin/src/dolphinmainwindow.h


"var_type var_name" for parameters



dolphin/src/dolphinmainwindow.cpp


Maybe add the (in this case unneeded) "> 0" check to make it more obvious 
when we enable/disable this action.



dolphin/src/dolphinmainwindow.cpp


Add one space before this, so that recentTabsMenu and this are justified



dolphin/src/dolphinmainwindow.cpp


space after ,



dolphin/src/dolphinrecenttabsmenu.h


"var_type var_name" for parameters


- Emmanuel Pescosta


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
> 
>

Re: 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
> > 
> >
> > 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
> 
>

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

[Thomas Lübking]

> On Juli 5, 2014, 11:36 vorm., Arjun Ak wrote:
> > dolphin/src/dolphinrecenttabsmenu.cpp, line 102
> > 
> >
> > Is it safe to 'delete' a QObject? shouldnt we be using deleteLater()

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.


- Thomas


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


On Juli 6, 2014, 10:46 vorm., Arjun Ak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118994/
> ---
> 
> (Updated Juli 6, 2014, 10:46 vorm.)
> 
> 
> 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
> 
>

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

[Arjun Ak]

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

(Updated July 6, 2014, 4:16 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 (updated)
-

  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

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

[Emmanuel Pescosta]

> On July 5, 2014, 2:15 p.m., Emmanuel Pescosta wrote:
> > Arjun, thanks for the updated patch!
> > 
> > Great so far, but looking at the code again, we should add a signal e.g. 
> > "closedTabsCountChanged(int)" to DolphinRecentTabsMenu and react on this 
> > signal in the main window. 
> > 
> > slot in main window:
> > closedTabsCountChanged(int count)
> > {
> > actionCollection()->action("undo_close_tab")->setEnabled(count > 0);
> > }
> > 
> > So we need no new member variable in DolphinMainWindow and we have only one 
> > place where we enable/disable "closed tabs" related actions.
> > 
> > At the end of DolphinRecentTabsMenu::handleAction we calculate the number 
> > of closed tabs first, then we can emit the signal 
> > "closedTabsCountChanged(int)".
> > So we can save a lot of changes (we need only add 2 slots and 1 signal).
> > 
> > Sorry that I didn't come up with this idea earlier, so you had to do a lot 
> > of extra work, my fault! :(

But only change your patch if you agree with me (this was just an idea) ;)


- Emmanuel


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


On July 5, 2014, 1:19 p.m., Arjun Ak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118994/
> ---
> 
> (Updated July 5, 2014, 1:19 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/dolphinui.rc 52826bb 
>   dolphin/src/dolphinrecenttabsmenu.h 34d4153 
>   dolphin/src/dolphinrecenttabsmenu.cpp a39f994 
>   dolphin/src/dolphinmainwindow.h acf60a4 
>   dolphin/src/dolphinmainwindow.cpp c60951d 
> 
> Diff: https://git.reviewboard.kde.org/r/118994/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Ak
> 
>

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

[Emmanuel Pescosta]

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


Arjun, thanks for the updated patch!

Great so far, but looking at the code again, we should add a signal e.g. 
"closedTabsCountChanged(int)" to DolphinRecentTabsMenu and react on this signal 
in the main window. 

slot in main window:
closedTabsCountChanged(int count)
{
actionCollection()->action("undo_close_tab")->setEnabled(count > 0);
}

So we need no new member variable in DolphinMainWindow and we have only one 
place where we enable/disable "closed tabs" related actions.

At the end of DolphinRecentTabsMenu::handleAction we calculate the number of 
closed tabs first, then we can emit the signal "closedTabsCountChanged(int)".
So we can save a lot of changes (we need only add 2 slots and 1 signal).

Sorry that I didn't come up with this idea earlier, so you had to do a lot of 
extra work, my fault! :(

- Emmanuel Pescosta


On July 5, 2014, 1:19 p.m., Arjun Ak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118994/
> ---
> 
> (Updated July 5, 2014, 1:19 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/dolphinui.rc 52826bb 
>   dolphin/src/dolphinrecenttabsmenu.h 34d4153 
>   dolphin/src/dolphinrecenttabsmenu.cpp a39f994 
>   dolphin/src/dolphinmainwindow.h acf60a4 
>   dolphin/src/dolphinmainwindow.cpp c60951d 
> 
> Diff: https://git.reviewboard.kde.org/r/118994/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Ak
> 
>

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

[Arjun Ak]

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



dolphin/src/dolphinrecenttabsmenu.cpp


Is it safe to 'delete' a QObject? shouldnt we be using deleteLater()


- Arjun Ak


On July 5, 2014, 4:49 p.m., Arjun Ak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118994/
> ---
> 
> (Updated July 5, 2014, 4:49 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/dolphinui.rc 52826bb 
>   dolphin/src/dolphinrecenttabsmenu.h 34d4153 
>   dolphin/src/dolphinrecenttabsmenu.cpp a39f994 
>   dolphin/src/dolphinmainwindow.h acf60a4 
>   dolphin/src/dolphinmainwindow.cpp c60951d 
> 
> Diff: https://git.reviewboard.kde.org/r/118994/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Ak
> 
>

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

[Arjun Ak]

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

(Updated July 5, 2014, 4:49 p.m.)


Review request for Dolphin and KDE Base Apps.


Changes
---

Fixed code style


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 (updated)
-

  dolphin/src/dolphinui.rc 52826bb 
  dolphin/src/dolphinrecenttabsmenu.h 34d4153 
  dolphin/src/dolphinrecenttabsmenu.cpp a39f994 
  dolphin/src/dolphinmainwindow.h acf60a4 
  dolphin/src/dolphinmainwindow.cpp c60951d 

Diff: https://git.reviewboard.kde.org/r/118994/diff/


Testing
---


Thanks,

Arjun Ak

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

[Arjun Ak]

> On July 2, 2014, 11:30 p.m., Emmanuel Pescosta wrote:
> > dolphin/src/dolphinmainwindow.cpp, lines 1136-1138
> > 
> >
> > Why?
> > 
> > This case should not happen, please add a Q_ASSERT instead

It would happen if the user cleared the history from the "Recently closed tabs" 
menu. Anyway it doesnt apply to the new patch


> On July 2, 2014, 11:30 p.m., Emmanuel Pescosta wrote:
> > dolphin/src/dolphinmainwindow.cpp, line 1139
> > 
> >
> > [] -> .at()

I never knew they were different, thought they were the same.


- Arjun


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


On July 5, 2014, 4:45 p.m., Arjun Ak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118994/
> ---
> 
> (Updated July 5, 2014, 4:45 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 acf60a4 
>   dolphin/src/dolphinmainwindow.cpp c60951d 
>   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
> 
>

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

[Arjun Ak]

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

(Updated July 5, 2014, 4:45 p.m.)


Review request for Dolphin and KDE Base Apps.


Changes
---

fixed issues


Bugs: 336818
http://bugs.kde.org/show_bug.cgi?id=336818


Repository: kde-baseapps


Description (updated)
---

Upon pressing CTRL+SHIFT+T, the most recently closed tab is reopened (with the 
help of Go->'Recently closed tabs' menu).


Diffs (updated)
-

  dolphin/src/dolphinmainwindow.h acf60a4 
  dolphin/src/dolphinmainwindow.cpp c60951d 
  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

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

[Emmanuel Pescosta]

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



dolphin/src/dolphinmainwindow.h


Move it to DolphinRecentTabsMenu



dolphin/src/dolphinmainwindow.cpp


Why?

This case should not happen, please add a Q_ASSERT instead



dolphin/src/dolphinmainwindow.cpp


[] -> .at()



dolphin/src/dolphinmainwindow.cpp


Please add a new method to DolphinRecentTabsMenu which returns true if 
there are closed tabs available for restoring and false otherwise, so that we 
get rid of all these size() < 3 checks in multiple places.

We can use the same method in DolphinRecentTabsMenu::handleAction to 
disable the recent tabs menu.



dolphin/src/dolphinmainwindow.cpp


KAction* undoCloseTab


- Emmanuel Pescosta


On July 2, 2014, 3:59 p.m., Arjun Ak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118994/
> ---
> 
> (Updated July 2, 2014, 3:59 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 acf60a4 
>   dolphin/src/dolphinmainwindow.cpp c60951d 
>   dolphin/src/dolphinui.rc 52826bb 
> 
> Diff: https://git.reviewboard.kde.org/r/118994/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Ak
> 
>

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

[Arjun Ak]

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

(Updated July 2, 2014, 7:29 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 (updated)
-

  dolphin/src/dolphinmainwindow.h acf60a4 
  dolphin/src/dolphinmainwindow.cpp c60951d 
  dolphin/src/dolphinui.rc 52826bb 

Diff: https://git.reviewboard.kde.org/r/118994/diff/


Testing
---


Thanks,

Arjun Ak

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

[Frank Reininghaus]

> On June 29, 2014, 2:50 p.m., Emmanuel Pescosta wrote:
> > Thanks for the patch!
> > 
> > I really like the idea to implement CTRL+SHIFT+T to reopen the previously 
> > closed tab, makes Dolphin more browser like :)
> > 
> > But please implement this feature in master, because:
> > 1. Dolphins frameworks branch is currently only for fixing KF5/Qt5 related 
> > bugs
> > 2. We changed quite a few recent tabs related lines (new 
> > DolphinRecentTabsMenu)
> > 
> > So please implement this behavior in DolphinRecentTabsMenu. Thanks!

If you only have a Qt5/KF5 build environment, and you don't want to set up a 
separate Qt4+KDE SC 4.x build (which would be understandable, because 
development there will stop at some point), we can also backport small patches 
from the frameworks branch to master for you. In most cases, small patches 
should apply to both branches with minor modifications. At least if master is 
fully merged into frameworks, which I have done a few minutes ago, so 
Emmanuel's DolphinRecentTabsMenu is also in frameworks now.

Thanks for your help!


- Frank


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


On June 28, 2014, 2:44 p.m., Arjun Ak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118994/
> ---
> 
> (Updated June 28, 2014, 2:44 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 1192f6e 
>   dolphin/src/dolphinmainwindow.cpp 7c9001b 
>   dolphin/src/dolphinui.rc 52826bb 
> 
> Diff: https://git.reviewboard.kde.org/r/118994/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Ak
> 
>

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

[Emmanuel Pescosta]

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


Thanks for the patch!

I really like the idea to implement CTRL+SHIFT+T to reopen the previously 
closed tab, makes Dolphin more browser like :)

But please implement this feature in master, because:
1. Dolphins frameworks branch is currently only for fixing KF5/Qt5 related bugs
2. We changed quite a few recent tabs related lines (new DolphinRecentTabsMenu)

So please implement this behavior in DolphinRecentTabsMenu. Thanks!

- Emmanuel Pescosta


On June 28, 2014, 4:44 p.m., Arjun Ak wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118994/
> ---
> 
> (Updated June 28, 2014, 4:44 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 1192f6e 
>   dolphin/src/dolphinmainwindow.cpp 7c9001b 
>   dolphin/src/dolphinui.rc 52826bb 
> 
> Diff: https://git.reviewboard.kde.org/r/118994/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Ak
> 
>