Add functionality to open accidentally closed tab from container's menu or using hotkey (Meta + T).
Details
Details
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
Looks good except what is noted below, thanks!
| sublime/container.cpp | ||
|---|---|---|
| 309 | why do you need the cast here? | |
| 365 | there is no point in using qobject_cast over static_cast if you don't check the result; not sure if you need to? | |
| sublime/mainwindow_p.cpp | ||
| 136 | I suggest Ctrl+Shift+T, like in Firefox for example. I think meta is weird for shortcuts, it is not used anywhere else. | |
Comment Actions
I like the change in general, only wonder whether we need to introduce a new queue for that - can't we reuse the recent file mechanism instead? or do people really want to have an infinite history here?
browsers btw have this feature too, but they probably rely on their history feature for this purpose, right?
| shell/mainwindow.cpp | ||
|---|---|---|
| 519 ↗ | (On Diff #12851) | dito |
| shell/mainwindow.h | ||
| 97 ↗ | (On Diff #12851) | remove space before & |
| shell/mainwindow_actions.cpp | ||
| 192 ↗ | (On Diff #12851) | dito |
| shell/mainwindow_p.h | ||
| 88 ↗ | (On Diff #12851) | dito |
| sublime/container.cpp | ||
| 124 ↗ | (On Diff #12851) | dito |
| 361 ↗ | (On Diff #12851) | remove space before ( |
| 367 ↗ | (On Diff #12851) | this could potentially end up storing tons of items... I wonder - could we instead rely on the "recent files" mechanism and only support reopening the last N items through that? |
| sublime/container.h | ||
| 22 ↗ | (On Diff #12851) | forward-declare instead of include |
| 88 ↗ | (On Diff #12851) | dito |
| sublime/mainwindow.cpp | ||
| 416 ↗ | (On Diff #12851) | dito |
| sublime/mainwindow.h | ||
| 144 ↗ | (On Diff #12851) | remove space before & |
| sublime/container.cpp | ||
|---|---|---|
| 367 ↗ | (On Diff #12851) | I did not find proper way to use "recent files", because this list is application wide and we need to track separate queue of closed files for each container. |
Comment Actions
Would be a useful additions, I know I would have used it before :)
| shell/mainwindow.cpp | ||
|---|---|---|
| 519 | Default argument values should be only in the declaration, no? At least my current and past compilers refuse to take such code, referring to -fpermissive. Hm, or only if both method declaration and definition have a default argument set? What is the purpose of a default argument value only in the definition? Especially when installed headers will miss that info bit about the default argument value? | |
Comment Actions
@coopht: Your points re recent files are very valid - let's not use it.
But one point makes me wonder: You said
I did not find proper way to use "recent files", because this list is application wide and we need to track separate queue of closed files for each container.
The hotkey is also application wide, no? So how is this supposed to work in a split view configuration? I assumed it would simply restore the file in the corresponding container?
Comment Actions
The hotkey is also application wide, no? So how is this supposed to work in a split view configuration? I assumed it would simply restore the file in the corresponding container?
Yes, It will restore files in corresponding container from container's queue.
Is it OK to limit queue by 100 entries ?
Comment Actions
What is the purpose of a default argument value only in the definition? Especially when installed headers will miss that info bit about the default argument value?I have never seen this before.
It's a mistake. I fixed it.
But the code was accepted by gcc-4.6.3
Comment Actions
Wouldn't it make sense to implement it somewhere separate as an observer rather than making the current sublime/container class even bigger?
Comment Actions
I don't think that these changes making sublime/container much bigger. This is a pretty small changes and the idea is to track closed files for each container.
What is the proper place to implement it as an observer?
Comment Actions
I moved it to ReopenAction.
If ReopenAction is used in shell/MainWindow or sublime/MainWindow than it become more complicated to maintain separate lists of recently closed files for each view.
I suggest to use ReopenAction in the container, to simplify support of separate views for each view.
I tried to minimize my changes to the container class.
| sublime/reopenaction.cpp | ||
|---|---|---|
| 34 ↗ | (On Diff #13173) | Why is it called action? It makes it look like it inherits QAction. Or actually, make it inherit it! note it's only instantiated once and then coupled with the action. |
| 49 ↗ | (On Diff #13173) | Why don't we just call ICore::self()->documentController()->openDocument(doc) here? We'll avoid all this tampering with the shell and sublime. It's what this all ends up doing anyway. |
| 63 ↗ | (On Diff #13173) | We should probably include more information than the url here, like the cursor position and even viewport. |
Comment Actions
Hi Alexander,
When I accidentally close a file, I use the existing action "Previous Visited Context". What you are implementing is only useful after accidentally closing several files. Is that what you want?
Comment Actions
Hi Alexander.
Yes, that what I want.
Also, if you close tab, which was not visited, you are unable to reopen it with "Previous Visited Context".
Comment Actions
Hi @coopht , still busy at work? :)
There has been no more activity on this WIP patch since more than a year. To keep our list of patches to review clean from dead ones, I would mark this review request as abandoned on Oct. 15th, unless there is an update. After that once there is some new version of the patch, it can be reactivated again.
Still looking forward to seeing you continue this patch :)
Comment Actions
Abandoning for now given inactivity by the original author, for cleaning the to-reivew list. Still hoping one day someone/you will pick up this again.