The combobox needs to be 2 pixels wider for contents to fit
ClosedPublic

Authored by antlarr on Oct 28 2016, 3:18 PM.

Details

Summary

When the size of the combo box adjusts to its contents, the dropdown
listbox isn't large enough to fit the contents and a vertical scrollbar
so instead of using Metrics::MenuButton_IndicatorWidth. I defined
a new Metrics::ComboBox_IndicatorWidth which is two pixels larger than
Metrics::MenuButton_IndicatorWidth.

To test this I used "kcmshell5 kcm_kscreen -style breeze" and opened
the resolutions QComboBox (If there are less than 15 possible
resolutions, a slider will appear instead of a QComboBox, so in that
case it's possible to use a virtual machine with QXL video that for
me shows plenty of possible resolutions).

Before this patch, most of the larger resolutions are shortened to
something like "192...200" or "160...200" and can't be read (in the
dropdown listbox, when selected they can be read correctly in the
closed combobox), after applying this patch, all the contents of the
listbox can be read correctly.

Diff Detail

Repository
R31 Breeze
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
antlarr updated this revision to Diff 7735.Oct 28 2016, 3:18 PM
antlarr retitled this revision from to The combobox needs to be 2 pixels wider for contents to fit.
antlarr updated this object.
antlarr edited the test plan for this revision. (Show Details)
antlarr added a reviewer: Plasma.
Restricted Application added a project: Plasma. · View Herald TranscriptOct 28 2016, 3:18 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson added inline comments.
kstyle/breezestyle.cpp
2507

you're still basing the height off the MenuButton_IndicatorWidth which you're otherwise not using

Is that intentional?

antlarr added inline comments.Oct 31 2016, 4:24 PM
kstyle/breezestyle.cpp
2507

Good point, I didn't notice that it was using MenuButton_IndicatorWidth to set the height, but still, height seems to be fine, so I wouldn't change that.

Hi,
I am fine with the commit (especially since it fixes a bug which I have had no time to reproduce)
Now since indeed 2 more pixels are needed on top of the ones from the MenuButton_IndicatorWidth, I would be inclided to just change
size.rwidth() += Metrics::ComboBox_IndicatorWidth into
size.rwidth() += Metrics::MenuButton_IndicatorWidth+2 (with a comment)
rather than adding a new enum in Metrics.
The idea with this enum is that you can in principle change all the values without breaking anything, which is not the case here.
The alternative would be to make the enum be:
ComboBox_IndicatorWidth = MenuButton_IndicatorWidth+2

antlarr updated this revision to Diff 7788.Oct 31 2016, 5:00 PM
  • Increase width by MenuButton_IndicatorWidth+2 instead of adding a new enum entry
This revision is now accepted and ready to land.Oct 31 2016, 5:01 PM
antlarr closed this revision.Oct 31 2016, 5:11 PM

@hpereiradacosta thanks for reviewing this. Can you cherry-pick the commit for Plasma/5.8?

@hpereiradacosta thanks for reviewing this. Can you cherry-pick the commit for Plasma/5.8?

done