Add expanding spacers as a customization option for toolbars
ClosedPublic

Authored by felixernst on Jul 21 2019, 11:49 AM.

Details

Summary

This commit adds spacers to the kxmlgui framework so all applications
using it will be able to have any amount of spacers in their toolbar(s).

KEditToolbar gets the --- spacer --- entry by default. This entry is
modified to allow any amount of spacers to be put into the toolbars
(just like separators).
The xml scheme is updated to allow "<Spacer name="spacer_0" />" nodes
(just like separators).
KXmlGuiBuilder then builds the simple spacer by itself.

Test Plan

Diff Detail

Repository
R263 KXmlGui
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
felixernst created this revision.Jul 21 2019, 11:49 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 21 2019, 11:49 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
felixernst requested review of this revision.Jul 21 2019, 11:49 AM
felixernst edited the test plan for this revision. (Show Details)Jul 21 2019, 11:53 AM
felixernst edited the summary of this revision. (Show Details)
felixernst edited the test plan for this revision. (Show Details)Jul 21 2019, 11:56 AM
felixernst planned changes to this revision.Jul 21 2019, 12:31 PM
This comment was removed by felixernst.

Use insertWidget(before, spacer) instead of addWidget(spacer)

felixernst edited the summary of this revision. (Show Details)Jul 21 2019, 12:50 PM
ngraham added subscribers: Frameworks, dfaure, VDG.
Restricted Application removed a subscriber: Frameworks. · View Herald TranscriptJul 21 2019, 4:16 PM

+100!

How about naming it "Flexible space" or "Expanding spacer" to make it a bit clearer?

Also, I would recommend doing most of the comment changes in a separate patch. They are good and useful, but not related to adding this awesome new spacer item.

dfaure accepted this revision.Jul 21 2019, 6:18 PM

A video showing the feature, in the merge request! I'm blown away!

Code looks fine.

src/kxmlguibuilder.cpp
349

(pre-existing, in the separator code above) QToolBar would be sufficient, technically, in this code.

This revision is now accepted and ready to land.Jul 21 2019, 6:18 PM

Ah yeah, about the name, I expected a fixed-width spacer until I saw the video. And maybe someone wants to provide that as well... so "Expanding spacer" would actually make sense for this feature.
Or maybe you even want to provide both right away, while at it...

+1 for adding both a fixed-width spacer as well as an expanding spacer.

You all are too kind!

Code looks fine.

First try \o/

I'll put the comments that aren't directly related into another revision.
I'll rename it to "--- expanding spacer ---" then. So I'll keep it lowercase and in the same style as "--- seperator ---" if nobody has a better idea.

fixed-width spacer

I don't really understand their benefit yet because I can't imagine a scenario where I would want one that wouldn't better be solved with an expanding one. So to me it seems like it is a widget we don't want to have so users don't pick the spacer that is worse in 95 % of cases out of lack of knowledge.
I can be convinced to add a fixed-width one though if I see an example where we would want them. We would have to decide what size a fixed-width spacer has.

felixernst retitled this revision from Add spacers as a customization option for toolbars to Add expanding spacers as a customization option for toolbars.Jul 22 2019, 10:28 AM

OK, if you don't think there is a use case, let's leave fixed-width spacers aside for now.

I was wondering abour the lowercase thing, too. It looks weird. Maybe Separator should become uppercase so they can both be?

felixernst updated this revision to Diff 62687.Sun, Jul 28, 5:25 PM
felixernst marked an inline comment as done.

Rename to "expanding spacer", Cast to QToolBar instead

Remove unrelated comments and documentation

felixernst added a comment.EditedSun, Jul 28, 5:29 PM

I kept the names lowercase because after trying both I liked them lowercase better. The visible difference between lowercase separator/spacer and title case actions makes the list a tiny bit more neat imo. I don't mind changing the names to title case though. Judge for yourself:

ngraham accepted this revision.Thu, Aug 1, 12:49 AM

I think we should make everything uppercase, and if the separators need visual differentiation, making them lowercase is kind of a hack.

However, that change would need to be made in a separate patch because it affects more than just the addition made here. So I recommend that in this patch, we follow the trend (lowercase) and discuss a potential casing change in another patch later. Let's not let this completely awesome new feature get delayed too long by bikeshedding. :)

This revision was automatically updated to reflect the committed changes.