Unify window and tab title
ClosedPublic

Authored by hallas on Jul 28 2018, 3:59 PM.

Details

Summary

Previously the title of tabs was a prettyfied version of the URL.
This is inconsistent with the title of the Window and in some cases
for specials URLs kind of misleading. This commit generalizes the
code from DolphinMainWindow so that both the DolphinMainWindow title
and the tab title uses the same function for the title. This also
means that the 'Show Full Path in Title Bar' also applies to the
tab title, and also that searches changes the tab title.

FEATURE: 387851

Test Plan

Open a new tab from the places panel and navigate around.

Diff Detail

Repository
R318 Dolphin
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1311
Build 1329: arc lint + arc unit
hallas created this revision.Jul 28 2018, 3:59 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptJul 28 2018, 3:59 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
hallas requested review of this revision.Jul 28 2018, 3:59 PM
hallas updated this revision to Diff 38662.Jul 28 2018, 4:06 PM

Correctly handle && in tab titles.

Change all '&&' to '&' so that the tab widget doesn't create keyboard
shortcuts, just like the original code did.

nicolasfella retitled this revision from FEATURE: 387851 to Unify window and tab title.Jul 28 2018, 4:10 PM
nicolasfella edited the summary of this revision. (Show Details)
markg requested changes to this revision.Jul 28 2018, 4:56 PM
markg added a subscriber: markg.

I think the logic you've chosen is wrong.
Lets take this fruit store analogy. You have a fruit store with an owner and fruit.
The fruit is dumb and can't do anything outside itself (this would be a tab in dolphin).
The owner (dolphinmainwindow) can look at fruit en change whatever it wants to change in the store (whole of dolphin).

What you did is make the fruit able to talk to the owner...

I think you should change this logic to add a "getCaption" to the tab logic. Then use m_activeViewContainer (i think you need to use that one) and ask the caption from the container. And call that function on each active container change.
That would be a sound logic in my book and keeps responsibility where it should be.
This would also prevent weird code constructs as you have now:

QString name = m_mainWindow->getCaption(url, currentTabPage() ? currentTabPage()->activeViewContainer() : nullptr);

which would become:

QString name = m_activeViewContainer->getCaption();
src/dolphintabwidget.h
199

Why would a tab need to know who it's parent is?
This is wrong imho.

This revision now requires changes to proceed.Jul 28 2018, 4:56 PM

I think the logic you've chosen is wrong.
Lets take this fruit store analogy. You have a fruit store with an owner and fruit.
The fruit is dumb and can't do anything outside itself (this would be a tab in dolphin).
The owner (dolphinmainwindow) can look at fruit en change whatever it wants to change in the store (whole of dolphin).

What you did is make the fruit able to talk to the owner...

I think you should change this logic to add a "getCaption" to the tab logic. Then use m_activeViewContainer (i think you need to use that one) and ask the caption from the container. And call that function on each active container change.
That would be a sound logic in my book and keeps responsibility where it should be.
This would also prevent weird code constructs as you have now:

QString name = m_mainWindow->getCaption(url, currentTabPage() ? currentTabPage()->activeViewContainer() : nullptr);

which would become:

QString name = m_activeViewContainer->getCaption();

Thanks for the feedback, and nice analogy :)

I tend to agree with you, when doing this change I was actually looking to place this code in a utility class, but I couldn't really find any other utility classes, so I ended up keeping the code as close to the original place as possible. But I think you are right that it doesn't really belong in the DolphinMainWindow class, so I will go ahead on fix up the commit.

hallas updated this revision to Diff 38675.Jul 28 2018, 6:22 PM

Moved the getCaption function from DolphinMainWindow to DolphinViewContainer.
Minor cleanups as a result of moving this function.

hallas updated this revision to Diff 38676.Jul 28 2018, 6:25 PM

Also remove the m_mainWindow member in DolphinTabWidget and revert the Constructor change.

hallas updated this revision to Diff 38677.Jul 28 2018, 6:27 PM

Also removes a couple of now unused includes.

hallas marked an inline comment as done.Jul 28 2018, 6:28 PM
hallas updated this revision to Diff 38678.Jul 28 2018, 6:29 PM

Also remove unused include from dolphintabwidget.cpp

markg resigned from this revision.Jul 28 2018, 9:30 PM

That looks much more sensible to me :)
Removing the -1.

Not a +1 yet though. I want to try this out locally first.

hallas added a comment.Aug 7 2018, 8:10 AM

That looks much more sensible to me :)
Removing the -1.

Not a +1 yet though. I want to try this out locally first.

Hey @markg - any update on this?

@ngraham - Could you also take a look at this? Is this the behavior we want for tab names?

ngraham requested changes to this revision.Aug 8 2018, 2:34 PM

Thanks for the ping, sometimes things get lost when everyone's busy!

Conceptually, I am in strong agreement with this. It makes sense that the tab title is the same as the window title.

However, this introduces a slight regression in tab names for certain use cases: KIOSlaves now have a trailing - / at the end of them. E.g. open a new tab and go to tags:// before and after applying your patch, then compare the tab titles. This is a pre-existing issue for the window title, but by using that window title for the tab title, we inherit the issue. The old tab title handling code handled that; do you think you could update the diff to handle it for the window title too so that the tab titles re-inherit the behavior?

This revision now requires changes to proceed.Aug 8 2018, 2:34 PM
hallas added a comment.Aug 8 2018, 5:57 PM

Thanks for the ping, sometimes things get lost when everyone's busy!

Conceptually, I am in strong agreement with this. It makes sense that the tab title is the same as the window title.

However, this introduces a slight regression in tab names for certain use cases: KIOSlaves now have a trailing - / at the end of them. E.g. open a new tab and go to tags:// before and after applying your patch, then compare the tab titles. This is a pre-existing issue for the window title, but by using that window title for the tab title, we inherit the issue. The old tab title handling code handled that; do you think you could update the diff to handle it for the window title too so that the tab titles re-inherit the behavior?

No worries :)

Thanks for the feedback, I will look into this issue and see if I can fix it.

So just to clarify the behavior; there should never be a - / in the window title or the tab title? This seems to be added specifically by line 416-421 and then appending the fileName or '/' if it is empty, so should this code simply be removed? What information is relevant for the user in this case? Maybe just showing the URL as the user has typed it is the most sane, so in the example you provide, the window title and tab title would simply be tags:// ? Personally I prefer this simple behavior of showing the short hand URL or the full path, depending on your settings (GeneralSettings::showFullPathInTitlebar).

Thanks for the ping, sometimes things get lost when everyone's busy!

Conceptually, I am in strong agreement with this. It makes sense that the tab title is the same as the window title.

However, this introduces a slight regression in tab names for certain use cases: KIOSlaves now have a trailing - / at the end of them. E.g. open a new tab and go to tags:// before and after applying your patch, then compare the tab titles. This is a pre-existing issue for the window title, but by using that window title for the tab title, we inherit the issue. The old tab title handling code handled that; do you think you could update the diff to handle it for the window title too so that the tab titles re-inherit the behavior?

No worries :)

Thanks for the feedback, I will look into this issue and see if I can fix it.

So just to clarify the behavior; there should never be a - / in the window title or the tab title? This seems to be added specifically by line 416-421 and then appending the fileName or '/' if it is empty, so should this code simply be removed? What information is relevant for the user in this case? Maybe just showing the URL as the user has typed it is the most sane, so in the example you provide, the window title and tab title would simply be tags:// ? Personally I prefer this simple behavior of showing the short hand URL or the full path, depending on your settings (GeneralSettings::showFullPathInTitlebar).

Personally I don't see what it's ever needed for; it's just noise. It's nice to show human-readable strings rather than technical jargon though. For example going to "tags://" should show "Tags", not "tags://".

hallas updated this revision to Diff 39351.Aug 9 2018, 12:44 PM

Removes the 'path - ' stuff from the window title. Previously Dolphin would use
'scheme/host - path' as the window title for non local URLs, this is not the
most user friendly thing to use. Instead the following algoithm is used for
non local URLs:

  • If fileName is non empty use that
  • If path is non empty and not "/" use that
  • If host is non empty use that
  • Use the URL

If GeneralSettings::showFullPathInTitlebar is true, then the full URL is used.

I would really like to have used the "Pretty Name" from the UDSEntry of the
current URL, but it is not easily available. I have tried to retrive it using
KIO::stat but not all protocols handle this very well, i.e. remote protocol.

markg added inline comments.Aug 9 2018, 2:51 PM
src/dolphinviewcontainer.cpp
428–446

I'm guessing now, please have a look at the code to see if this is possible.

You're doing QUrl foo to figure stuff out about the url. It will (probably) work in most scenario's but there is an easier and more reliable way.
When a view is opened or changed, it - somewhere in dolphin - exists as KFileItem. That object knows everything about the item (if it's a folder, a file, a link.. etc...).

Now, i don't see the KFileItem as class member of DolphinViewContainer so i'm guessing it comes in DolphinViewContainer via signals/slots (where i do see it) but isn't stored. If you want to go for a reliable title handling i'd go for saving the KFileItem that belong to the current view as a class member and build up the title based on that.

You should not need to stat for it again as that is already done when the item is opened. You just need to keep it stored in DolphinViewContainer to use it.

It will probably simplify this getCaption function quite a bit.

markg added a comment.Aug 9 2018, 2:56 PM

I would really like to have used the "Pretty Name" from the UDSEntry of the
current URL, but it is not easily available. I have tried to retrive it using
KIO::stat but not all protocols handle this very well, i.e. remote protocol.

I just said it in my other comment, but you should not have a need to do that.
Somehow somewhere this view is updated with a new path. The point where it does that is either a QUrl or a KFileITem (looks like a QUrl..) The calling side must have a KFileItem of it so passing that instead would make sense to me.

hallas added inline comments.Aug 10 2018, 11:52 AM
src/dolphinviewcontainer.cpp
428–446

Hi Mark,

yes I would really like to avoid this QUrl magic :) The problem (as I see it), is that I do not always have a KFileItem with the correct contents (but please correct me if I am wrong here, since this code is pretty new to me), for example: When Dolphin starts up, it basically navigates to a QUrl either passed on the command line, or uses the home directory, so in this case we do not have a populated KFileItem. But when the user navigates using the view, we should have a KFileItem, because the view basically displays a list of KFileItems, so no problem here. But if the user types in a new url in the locator, i.e. if I type in remote:// this I do not have a KFileItem for this URL and hence we cannot display the user friendly string for it. This is why i tried doing a KIO::stat on the QUrl, but had limited success. It seems like some of the "pseudo" protocols like remote:// and smb:// doesn't really support stat in all cases, for example stat of remote: doesn't give me the "Network" pretty name :/

So is there a way where I consistently can populate KFileItem, mainly the UDS_NAME or UDS_PRETTY_NAME attributes, given a QUrl?

markg added inline comments.Aug 10 2018, 7:44 PM
src/dolphinviewcontainer.cpp
428–446

When you press <enter> dolphin does it's magic and opens the url you typed. At that point a KFileITem of it lives somewhere within Dolphin's reach otherwise no view would be populated ;)

I will have a look at this tomorrow and see if i can figure something out.

Funny note though, the very first (or one of) commits i did in dolphin was unifying the title to what it is now :)
That was already an improvement over how it was, you just make it much better, hehe.

markg added a comment.Aug 11 2018, 4:22 PM

Oke, that is quite a bit of layers to figure out...
DolphinView::rootItem, which you can access in DolphinViewContainer::getCaption as m_view->rootItem(); is what you want to use.

But i've got bad news as well.
You update the window title _before_ the KIO model had time to do it's request so the m_view->rootItem(); is still invalid (or the previous folder).

You need to defer that call till after the model had set the root url (m_view->rootItem() ultimately comes from KCoreDirLister::rootItem). It is set when KCoreDirLister::listDir is called.
That could lead to the title not being set till _after_ all files have been received. Which would be rather ugly as the title only changes after receiving files.

A possible fix for you would be to listen for the KCoreDirLister::started signal, but that object alone is hidden quite deeply (It's a member of KFileItemModel) and i don't even know if that would work. You're best of making a small test project to test this out.
Also, if that is not working, it would probably be accepted as a patch to have the rootitem set when started is emitted. That would be a patch to KCoreDirLister.

I hope this helps. However you fix it, there is no easy way :)

Please try to write unit test, if you can (have a look at dolphinmainwindowtest.cpp). That's the only way to make sure we don't break anything :)

A possible fix for you would be to listen for the KCoreDirLister::started signal, but that object alone is hidden quite deeply (It's a member of KFileItemModel) and i don't even know if that would work. You're best of making a small test project to test this out.
Also, if that is not working, it would probably be accepted as a patch to have the rootitem set when started is emitted. That would be a patch to KCoreDirLister.

To answer myself.
Don't bother listening for the KCoreDirLister::started signal and then getting the rootitem. It's not set yet at that point. Just tried that out.

A possible fix for you would be to listen for the KCoreDirLister::started signal, but that object alone is hidden quite deeply (It's a member of KFileItemModel) and i don't even know if that would work. You're best of making a small test project to test this out.
Also, if that is not working, it would probably be accepted as a patch to have the rootitem set when started is emitted. That would be a patch to KCoreDirLister.

To answer myself.
Don't bother listening for the KCoreDirLister::started signal and then getting the rootitem. It's not set yet at that point. Just tried that out.

Hi Mark,

thanks for trying this out :)

I have been digging a bit more into this myself, and haven't found a solution. It seems to be a bit of a fundamental problem, basically Dolphin doesn't have a KFileItem in all cases, and it has not always a way to get a KFileItem for a given QUrl. A simple example is, if you click 'Network' in the places panel, the DolphinMainWindow class is notified with a QUrl pointing to the place i clicked, in this case this will be 'remote://', but Dolphin has no way to query the 'Remote' KIO to get the display string ('Network') from it, so it cannot display this information. Today this situation is covered by querying the places model if a QUrl matches a places item, and if so use the display text for that. When Dolphin displays the contents of 'remote://' i see for example 'Samba Shares' which actually points to 'smb://', but the KFileItem::name returns 'Samba Shares', in this case where Dolphin has done KIO::listDir on 'remote://' we get the information we need. But in the case where the user entered 'smb://' in the location bar, Dolphin has no way to get a KFileItem for 'smb://' because that is returned by the 'remote://' protocol, and it has no way of knowing this coupling. This is the situation where i tried using KIO::stat on 'smb://' but that doesn't return this information, since it is the 'remote://' protocol implementing this.

So long story short, with the current KIO infrastructure I can't really see a way where we always use the window title 'Samba Shares' if Dolphin has 'smb://' as the current url, we could achieve this in some cases, but it would be rather inconsistent.

Currently I think I am mostly for keeping this patch as is and discuss if this window title stuff should work in a fundamentally different way?

What do you guys think? Guidance is very much welcome :D

I'm pretty much okay with the current approach. @markg, thoughts?

markg added a comment.Aug 22 2018, 1:47 PM

I'm pretty much okay with the current approach. @markg, thoughts?

I agree, it's the best possible way at the moment.
+1, wait for another +1 before shipping though.

markg accepted this revision.Aug 22 2018, 1:48 PM
ngraham accepted this revision.Aug 22 2018, 6:39 PM
This revision is now accepted and ready to land.Aug 22 2018, 6:39 PM
hallas closed this revision.Aug 24 2018, 10:09 AM
markg added a comment.EditedAug 25 2018, 10:28 AM

Closed?
What's going on in here?

Edit

Ah, i think i see it. @hellas committed on behalf of the author.
Why didn't phab pick that up as this commit and make a mention of it? It should do that, right?

Phab seemed to notice the commit up top, but it didn't notice down here in the history. Odd.

elvisangelaccio reopened this revision.Aug 26 2018, 2:04 PM

Has anyone actually tried this patch? If I right-click my Trash place and I open it in a new tab, the new tab has the trash icon but the current (!) window title as tab title. I'm pretty sure this is not what we want.

Removes the 'path - ' stuff from the window title. Previously Dolphin would use
'scheme/host - path' as the window title for non local URLs, this is not the
most user friendly thing to use. Instead the following algoithm is used for
non local URLs:

  • If fileName is non empty use that
  • If path is non empty and not "/" use that
  • If host is non empty use that
  • Use the URL

    If GeneralSettings::showFullPathInTitlebar is true, then the full URL is used.

    I would really like to have used the "Pretty Name" from the UDSEntry of the current URL, but it is not easily available. I have tried to retrive it using KIO::stat but not all protocols handle this very well, i.e. remote protocol.

This description should be part of the commit message, not hidden among phabricator comments.

src/dolphintabwidget.h
194

Please fix the apidox, there is no url argument anymore.

src/dolphinviewcontainer.h
144

We don't use getXXX() for getters in Qt/KDE, just call it caption().

This revision is now accepted and ready to land.Aug 26 2018, 2:04 PM
elvisangelaccio requested changes to this revision.Aug 26 2018, 2:05 PM
This revision now requires changes to proceed.Aug 26 2018, 2:05 PM

Indeed, you're right @elvisangelaccio. I tested the menu item and keyboard shortcut, but didn't test the Open in New Tab context menu item, oops. :(

@hallas can you please submit a patch that fixes the issue of tab titles having the wrong name when opened from the Open in New Tab context menu item as well as Elvis's other concerns? Thanks!

hallas added a comment.Sep 3 2018, 9:26 AM

I have fixed the issue that my patch introduced in D15112 - so is there something else I need to do here?

Thanks for the feedback btw :)

elvisangelaccio accepted this revision.Sep 4 2018, 7:59 PM

Thanks for the fixes!

This revision is now accepted and ready to land.Sep 4 2018, 7:59 PM
ngraham closed this revision.Sep 4 2018, 8:00 PM

Apparently this also caused https://bugs.kde.org/show_bug.cgi?id=398817

@hallas Could you take a look please?