Review Request 110674: Moving KToolBarSpacerAction to kde4support

[David Faure]

> On May 27, 2013, 11:02 p.m., David Faure wrote:
> > staging/kde4support/src/kdeui/ktoolbarspaceraction.h, line 31
> > <http://git.reviewboard.kde.org/r/110674/diff/2/?file=146722#file146722line31>
> >
> >     @deprecated since 5.0, use XXX instead.
> >     
> >     What should the k3b, amarok, and kopete people use instead? They need to know...
> >     
> >     QToolBar::addSeparator? It doesn't have width/minWidth/maxWidth stuff.
> >     
> >     Hmm, OK, going through the uses of KToolBarSpacerAction in lxr.kde.org seems to indicate that none of the apps set these things anyway, so maybe addSeparator is enough indeed.
> >     
> >     Anyway - please always add @deprecated since 5.0 and the suggested replacement, for the benefit of people porting the code.
> 
> Aleix Pol Gonzalez wrote:
>     This is tricky, QToolBar does a different thing, it adds a visible separator in the toolbar instead of adding a space as KToolBarSpacerAction is doing.
>     
>     Maybe this class should stay?
> 
> David Faure wrote:
>     I'm not sure it's worth it. If it is, the right place would be in Qt, but... hard to justify, possibly?
>     
>     I think the best thing to do would be to talk to the k3b developers (they seem to be the main users of that class; otherwise amarok and kopete), and see if they really think this should exist (in KF5 or Qt). It could be that this predates QToolBar::addSeparator or that they didn't see it because they looked in kdelibs first.
> 
> Aleix Pol Gonzalez wrote:
>     ** See screenshots **
>     
>     As far as I can see, QToolBar::addSeparator and the KToolBarSpacerAction do different things. We can argue that the KToolBarSpacerAction is not needed. For example in k3b, the spacer is used to push the configuration button to the right corner. It's arguable that it's desirable, but they do different things.

Ah, this is about right-alignment, not just a bit of fixed space. I see.

Interesting. Back when konqueror had a right-aligned toolbar button it didn't use that action. KonqLogoAction::createWidget was creating a container widget with a QHBoxLayout, addStretch, and then add the actual button (git show 3ee12e3dadb to see the code). Not as convenient, of course, but can be done with "standard Qt API".

OK. Maybe all the unused minimumWidth/maximumWidth/width API should be removed from this class, and it should be clearly documented as "a stretch action, which ensures that every action after this one is right-aligned in the toolbar". In the long run we can look into getting this into Qt, but it's not critical at this point, so I would say, let's move it to kwidgetaddons.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110674/#review33247
-----------------------------------------------------------


On June 3, 2013, 2:17 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110674/
> -----------------------------------------------------------
> 
> (Updated June 3, 2013, 2:17 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> -------
> 
> Just moves the file, nobody seems to be using it in kdelibs anyway.
> 
> 
> Diffs
> -----
> 
>   staging/kde4support/src/kdeui/ktoolbarspaceraction.h 32e8fb2 
> 
> Diff: http://git.reviewboard.kde.org/r/110674/diff/
> 
> 
> Testing
> -------
> 
> Builds and installs fine.
> 
> 
> File Attachments
> ----------------
> 
> ::addSeparator()
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/06/03/k3b-nospaceraction.png
> KToolBarSpacerAction
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/06/03/k3b-spaceraction.png
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130603/c8a42a83/attachment-0001.html>