Fix incorrect Kickoff tab bar layout for vertical panels
ClosedPublic

Authored by lisin on Aug 7 2019, 10:55 AM.

Details

Summary

This fixes the broken layout of the tab bar (tab bar takes the whole view)
when a panel is changed from horizontal to vertical that persists
until plasmashell is restarted.
BUG: 393888

Test Plan

Change panel orientation from horizontal to vertical. Open Kickoff.
Before fix: tab bar fills the whole view making the Kickoff unusable even if you make the panel horizontal again.
After fix: tab bar has the correct size.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
lisin created this revision.Aug 7 2019, 10:55 AM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 7 2019, 10:55 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
lisin requested review of this revision.Aug 7 2019, 10:55 AM

Line onHeightChanged: onWidthChanged() fixes BUG: 395390
I'm not sure if this is the best solution. For some reason, plasmacomponents/qml/TabBar.qml lacks an onHeightChanged() function but it has onWidthChanged() that seems to do what needs to happen here. Maybe TabBar.qml should be changed instead.

The rest of the changes is for BUG: 393888
It seems to me that the removed code was a more clean way to do this, but it didn't update the values without restarting.

lisin added a comment.Aug 7 2019, 11:21 AM

I also couldn't reproduce the gray overlay (which is caused by tabBarSeparator having a wrong size and taking the whole view - yesterday I could reproduce it but no more) that is shown in the comment 16 here: https://bugs.kde.org/show_bug.cgi?id=395390#c16
So it may still be present. Currently, I can't see any separator but it's a different issue.

lisin edited the summary of this revision. (Show Details)Aug 7 2019, 11:25 AM
ngraham accepted this revision.Aug 7 2019, 3:53 PM

Thanks very much for the patch! This fixes both issues for me and looks conceptually like an appropriate fix to me, but I'm not the original author or maintainer of this code as @hein is, so let's wait for his review.

This revision is now accepted and ready to land.Aug 7 2019, 3:53 PM
hein added a comment.Aug 8 2019, 2:44 AM

... I'm not the author or maintainer of this code, but I had a look anyway :).

@lisin, I agree with you that the sizing bug should be fixed in TabBar instead, in plasma-frameworks.git. It's very unorthodox to call a property change handler as a function, and it's not going to fix this for other potential users of the component.

Otherwise the patch looks good. Could you resubmit it without line 435, and then submit a seperate patch to plasma-frameworks?

lisin updated this revision to Diff 63369.EditedAug 8 2019, 5:56 PM
lisin edited the summary of this revision. (Show Details)
lisin edited the test plan for this revision. (Show Details)

Removed line 435. I'll try to submit a separate patch for TabBar.

Thanks very much!

This revision was automatically updated to reflect the committed changes.

Now we just need that plasma-frameworks patch to also fix https://bugs.kde.org/show_bug.cgi?id=395390. :)

Let me know if you need a hand.

lisin added a comment.Aug 9 2019, 2:01 AM

Thank you, this was my first ever contribution to FOSS!
I have submitted the fix for the TabBar here: https://phabricator.kde.org/D23036
I'm not quite sure about who to add as reviewers though.

Very nice job! May it be the first of many. :) I hope the process wasn't too difficult. We're trying to make it smoother for new contributors.

lisin added a comment.Aug 9 2019, 2:26 AM

Yes, getting into it was pretty straightforward and quick, KDE team did a great job!