Make tabs rename-able
Needs ReviewPublic

Authored by krutovmikhail on Mar 25 2019, 10:39 PM.

Details

Reviewers
ngraham
elvisangelaccio
Group Reviewers
Dolphin
Summary

FEATURE: 197009
FIXED-IN: 19.08.0

Test Plan

Right click - rename tab, enter some name to rename tab

Right click - rename tab, clear lineedit in dialog, ok to reset name to default

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D20052
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10170
Build 10188: arc lint + arc unit
krutovmikhail created this revision.Mar 25 2019, 10:39 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMar 25 2019, 10:39 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
krutovmikhail requested review of this revision.Mar 25 2019, 10:39 PM
ngraham retitled this revision from BUG: 197009 - Enchancement: Add tab names to Make tabs rename-able.Mar 26 2019, 12:47 PM
ngraham edited the summary of this revision. (Show Details)
ngraham requested changes to this revision.Mar 26 2019, 1:08 PM
ngraham added a subscriber: ngraham.

Thanks, this generally works very nicely. I found a big bug though: when you rename a tab and then move it to another position on the tab bar, the tab in its old position no longer updates its name when the user navigates around in that tab, and the renamed tab inappropriately does update its name as the user navigates around.

src/dolphintabbar.cpp
166

Spaces around the equals sign

168

"Rename Tab" (use title case for window titles)

src/dolphintabwidget.cpp
33

Don't want this in production code

src/dolphintabwidget.h
176

Unnecessary whitespace change

This revision now requires changes to proceed.Mar 26 2019, 1:08 PM

Thanks for the review.

Note to self:

Also tab detaching, tab closing and check if other operations on tabs exist.

  • index -> uuid for tab identification
krutovmikhail marked 4 inline comments as done.
  • index -> uuid for tab identification
  • index -> uuid for tab identification

I've updated the diff, fixing both the issue with tab movement and styling.

I don't know if it would be a good idea to add a parameter (--tabname or something) to main.cpp to allow passing the tab name after detachement. What do you think?

I've updated the diff, fixing both the issue with tab movement and styling.

I don't know if it would be a good idea to add a parameter (--tabname or something) to main.cpp to allow passing the tab name after detachement. What do you think?

Nah, I don't think that's needed because Dolphin doesn't let you change the window title in the first place.

ngraham accepted this revision.Mar 26 2019, 9:00 PM
ngraham added a subscriber: elvisangelaccio.

This looks good to me now! Nice feature. @elvisangelaccio?

This revision is now accepted and ready to land.Mar 26 2019, 9:00 PM
elvisangelaccio requested changes to this revision.Mar 26 2019, 9:28 PM
elvisangelaccio added inline comments.
src/dolphintabbar.cpp
23

Unused, please remove it.

166

We could use a more descriptive variable name here, for example renamed or similar.

167

Please add const

168

Maybe we can add a colon at the end of "New tab name" ?

src/dolphintabpage.h
143

Coding style: please call it uuid(), we don't usually use the getXXX naming style for getters.

src/dolphintabwidget.cpp
38

Not needed, the constructor is implicitly called for QMap objects.

280

Coding style: brace should go to next line for functions.

286

Coding style: else starts at the end of previous line.

This revision now requires changes to proceed.Mar 26 2019, 9:28 PM
krutovmikhail marked 8 inline comments as done.
  • FEATURE: 197009 Implement tab renaming in dolphin
hallas added a subscriber: hallas.Mar 30 2019, 6:42 AM
hallas added inline comments.
src/dolphintabwidget.cpp
121–126

Maybe this logic should be moved to the tabName function instead?

src/dolphintabwidget.h
226

Why is this information stored here and not in the individual Tab Pages? If a Tab Page knew it's own name, you wouldn't need the UUID stuff so it could simplify the code quite a bit.

cfeck added a subscriber: cfeck.Apr 16 2019, 7:19 PM

Do you plan to make (the requested) changes? If yes, please change the status.

meven added a subscriber: meven.Jan 13 2020, 6:46 PM

ping
If somebody finds it useful I may commandeer this.

@krutovmikhail are you okay with that?

Can we please move this to gitlab and update bug 197009, thanks.

@meven has this been moved to commandeered and moved to Gitlab? I think you can safely do so if you haven't as the original author never replied.