New tab should be placed after the current tab
ClosedPublic

Authored by hallas on Feb 12 2019, 10:08 AM.

Details

Summary

When opening a new using the context menu the new tab should be placed
after the currently open tab, not at the end of the tab list.

BUG: 403690

Test Plan

Open multiple tabs and select a different tab than the last one.
Open a new tab using the context menu and see that it opens after the currently
selected tab.
Open a tab using Ctrl+T and see that it opens at the end.

Diff Detail

Repository
R318 Dolphin
Branch
open_in_new_tab_after_current_tab
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8408
Build 8426: arc lint + arc unit
hallas created this revision.Feb 12 2019, 10:08 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptFeb 12 2019, 10:08 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
hallas requested review of this revision.Feb 12 2019, 10:08 AM
ngraham requested changes to this revision.Feb 12 2019, 6:16 PM
ngraham added a subscriber: ngraham.
ngraham added inline comments.
src/dolphintabwidget.h
124–125

In general, using bools as arguments is not ideal since it's not very readable. Consider defining an Enum instead.

For further information see for example:

This revision now requires changes to proceed.Feb 12 2019, 6:16 PM
hallas updated this revision to Diff 51571.Feb 13 2019, 7:59 AM

Change DolphinTabWidget::openNewTab to take in an enum instead of a bool.

hallas marked an inline comment as done.Feb 13 2019, 8:01 AM

@ngraham - do you have any comments for this from a usability point of view?

src/dolphintabwidget.h
124–125

Good point! I have created an enum instead.

ngraham accepted this revision.Feb 13 2019, 3:28 PM
ngraham added a subscriber: elvisangelaccio.

It's great! Very usable--a bit improvement, in my eyes. This is the behavior that web browsers use too and I think it's very humane.

@elvisangelaccio, are you good with this?

This revision is now accepted and ready to land.Feb 13 2019, 3:28 PM
elvisangelaccio requested changes to this revision.Feb 16 2019, 11:26 AM
elvisangelaccio added inline comments.
src/dolphintabwidget.cpp
161–168

How about a simpler if?

int newTabIndex = -1;
if (tabPlacement == AfterCurrent) {
    newTabIndex = currentIndex() + 1;
}

(we can always make it a switch if we add new values to the enum, which is unlikely).

src/dolphintabwidget.h
42

How about AfterCurrentTab and AfterLastTab ? The underscore is unusual in camelcase names.

This revision now requires changes to proceed.Feb 16 2019, 11:26 AM
hallas updated this revision to Diff 51863.Feb 16 2019, 2:30 PM
hallas marked 3 inline comments as done.

Change TabPlacement enum names.
Simplify the code that calculates the index of the new tab.

hallas added inline comments.Feb 16 2019, 2:30 PM
src/dolphintabwidget.cpp
161–168

Yes this is a bit simpler :)

src/dolphintabwidget.h
42

Good point, I have changed the names

elvisangelaccio accepted this revision.Feb 16 2019, 3:22 PM

Please use FEATURE: instead of BUG: in the commit message.

Do you have commit access?

This revision is now accepted and ready to land.Feb 16 2019, 3:22 PM
hallas closed this revision.Feb 16 2019, 3:51 PM

Please use FEATURE: instead of BUG: in the commit message.

Do you have commit access?

Sorry, I didn't see your comment to after I had landed the change :) Hope it is ok anyway.

Please use FEATURE: instead of BUG: in the commit message.

Do you have commit access?

Sorry, I didn't see your comment to after I had landed the change :) Hope it is ok anyway.

Don't worry, just remember it for the next time :)