New tabs should be placed after the currently active tab when using
middle click.
Details
Details
- Reviewers
ngraham elvisangelaccio - Group Reviewers
Dolphin - Commits
- R318:e602e532c0f7: New tab placed after current tab when middle-clicking
Open new tab from the places panel using middle click, verify that the
Open new tab from the folders panel using middle click, verify that the
tab is opened after the current tab
Open new tab by middle clicking on the Back button, verify that the tab
is opened after the current tab
Open new tab by middle clicking on the Forward button, verify that the tab
is opened after the current tab
FEATURE: 403690
Diff Detail
Diff Detail
- Repository
- R318 Dolphin
- Branch
- open_new_tab_after_current_tab_middle_click (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 9107 Build 9125: arc lint + arc unit
| src/dolphintabwidget.h | ||
|---|---|---|
| 33 ↗ | (On Diff #52209) | @elvisangelaccio - What do you think about having this enum class placed here? Should it have it's own file or is this fine? |
Comment Actions
Thanks for sending a follow-up patch so quickly!
I tried out your test plan (thanks for that!) and everything works but one thing: if I middle-click the back button, the new tab is opened at the end, not after the current tab.
Comment Actions
Thanks, much better. Putting the DolphinTabPlacement class in dolphintabwidget makes sense to me, but make sure @elvisangelaccio agrees. :)
| src/dolphintabwidget.h | ||
|---|---|---|
| 33 ↗ | (On Diff #52209) | @elvisangelaccio - do you have any comments for this? |
| src/dolphinmainwindow.h | ||
|---|---|---|
| 51 | This is a bit weird, never seen forward declarations used for an enum. Why not just #include "dolphintabwidget.h" ? | |
| src/dolphintabwidget.h | ||
| 33 ↗ | (On Diff #52209) | No it's fine here. But why make it an enum class? Is it because you wanted to forward declare it? If yes, see my comment above. |
| src/dolphinmainwindow.h | ||
|---|---|---|
| 51 | We could, I was just trying to minimize the amounts of includes in the header file, I guess that is also the reason for the other forward declarations? One of the arguments for using enum class is specifically that you can forward declare it, it is technically also possible with an unscoped enumeration but you can't do it in all situations. | |
| src/dolphintabwidget.h | ||
| 33 ↗ | (On Diff #52209) | Forward declaration is one argument, but also that you get a scope so that the individual enumerators doesn't name clash with anything else. Does KDE have any general guidelines for using/not using this? Should I change it? |
| src/dolphinmainwindow.h | ||
|---|---|---|
| 51 |
Those are fine because you don't need to include the whole header if you just need a pointer to the object. | |
| src/dolphintabwidget.h | ||
| 33 ↗ | (On Diff #52209) | The clash protection is a good argument, but then it's an unrelated change that should go in another commit (and there are many other old-style enums in dolphin which should be ported). I don't think we have guidelines about enums in KDE, but using enum class for new code makes sense of course. |
| src/dolphintabwidget.h | ||
|---|---|---|
| 33 ↗ | (On Diff #52209) | I have reverted this part of the change, so keeping the enum as an enum and as a type within DolphinTabWidget and then include it in the headers that now require it - hope that is ok :) |
Comment Actions
Create dedicated functions in DolphinMainWindow for opening a tab after the current tab and as the last tab.