D14442: Unify window and tab title
David Hallas
noreply at phabricator.kde.org
Sat Jul 28 19:07:51 BST 2018
hallas added a comment.
In D14442#299644 <https://phabricator.kde.org/D14442#299644>, @markg wrote:
> 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.
REPOSITORY
R318 Dolphin
REVISION DETAIL
https://phabricator.kde.org/D14442
To: hallas, #dolphin, ngraham, elvisangelaccio, markg
Cc: markg, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180728/2dedb84c/attachment.htm>
More information about the kfm-devel
mailing list