Split Style & Helper into files by widget type
AbandonedPublic

Authored by ndavis on Nov 26 2019, 11:21 AM.

Details

Summary

The goal of this change is to make it easier for people to hack on the Breeze KStyle by making it easier to find which parts they need to change.

No visual changes were made.

Test Plan

test with oxygen-demo5 and kuiviewer untitled.ui

Diff Detail

Repository
R31 Breeze
Branch
ndavis/kstyle-refactor (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19196
Build 19214: arc lint + arc unit
ndavis created this revision.Nov 26 2019, 11:21 AM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 26 2019, 11:21 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ndavis requested review of this revision.Nov 26 2019, 11:21 AM
ndavis added a comment.EditedNov 26 2019, 11:22 AM

Please tell me if something about the structure does not make sense to you or if you think improvements could be made.

ndavis edited the summary of this revision. (Show Details)Nov 26 2019, 11:30 AM
ndavis edited the test plan for this revision. (Show Details)

Great, navigating in Breeze source will be a lot simpler now 👍

Very nice, I applaud the goal. However it looks like on net a lot more lines of code were added than were removed, which suggests the addition of redundant boilerplate or unrelated style refactoring. In the former case, can we factor out some of it into additional helper functions?

Very nice, I applaud the goal. However it looks like on net a lot more lines of code were added than were removed, which suggests the addition of redundant boilerplate or unrelated style refactoring. In the former case, can we factor out some of it into additional helper functions?

I didn't add any new lines of code, I just moved them into other files. I did format the new files in widgets/, but not the existing files.

ndavis updated this revision to Diff 70380.Nov 26 2019, 10:15 PM
  • Move renderSidePanelFrame and renderSelection into itemview.cpp
  • Move renderDecorationButton into mdi.cpp
ndavis added a comment.EditedNov 26 2019, 10:24 PM

I plan to reformat the existing files with the KF5 code style in another patch. Right now, it's very KDE4-like

Ah whoops, I missed the huge kstyle/breezestyle.cpp diff because it was collapsed.

Conceptually this makes sense to me and I can spot no visual regressions resulting from it. No formal acceptance from me yet because I feel like this requires more eyes, and we should probably keep it open for a while to make sure everything has gotten moved properly.

Great work though! I think this will really make it easier to contribute to Breeze.

ndavis updated this revision to Diff 70382.Nov 26 2019, 11:01 PM
  • Rename combobox.h and tabbar.h to be like private headers

They use the BreezePrivate namespace, so it seemed appropriate.

ndavis updated this revision to Diff 70384.Nov 26 2019, 11:07 PM
  • Actually rename all occurrences of combobox.h and tabbar.h
hpereiradacosta added a comment.EditedNov 27 2019, 3:41 AM

You are missing copyright information and license in all the newly created files.
On the review side: it is impossible to actually review, right ?

As for the conceptual side: I fear this is addressing a non existing issue, and giving a wrong impression about how one should hack on a widget style. It is wrong to think that you can hack on a widget style widget by widget without consideration about how they should appear one with respect to the others, how they should align one with respect to the other, and how the other widgets are implemented. You do need to know the whole code and interplay before starting to hack anyway. The splitting does not change this. In the end it might just result in a lot of duplicated code.
But I wont prevent you for pushing this. Who does the job decides. After all, none of the above is a strong objection. I just do not think it makes hacking breeze easier.

Hugo

Also (and somewhat independently from this patch): with Qt6/KF6 being around the corner, I somewhat wonder about the utility of rewritting and hacking breeze at that time. Would this make sense to actually leave breeze (which is a rather well-working theme, with very little bug reports) unchanged, and start working anew on a new widget style targeting KF6 ? Would that not be more exciting, bring more people in, trigger new ideas, give more freedom ? And be a more ambitious task all in all ?
One could of course start from breeze, and apply all the new ideas about how the code should be organized, how highlight should look, and checkboxes, without disturbance for something which has lived largely unchanged (and with not so many complains) for all the kf5 lifetime ...

My two cents.

You are missing copyright information and license in all the newly created files.

Thanks, should I just copy/paste the info from breezestyle.cpp?

On the review side: it is impossible to actually review, right ?

I was kind of worried that might be the case.

As for the conceptual side: I fear this is addressing a non existing issue, and giving a wrong impression about how one should hack on a widget style. It is wrong to think that you can hack on a widget style widget by widget without consideration about how they should appear one with respect to the others, how they should align one with respect to the other, and how the other widgets are implemented. You do need to know the whole code and interplay before starting to hack anyway. The splitting does not change this. In the end it might just result in a lot of duplicated code.

I have seen others in the VDG chat room say that the massive length of breezestyle.cpp made hacking on Breeze too intimidating. I understand your concern about giving the wrong impression though. I'm hoping to use these new files as a way of categorizing by visible widget types ("Where is the code for tabs? In tabbar.cpp", not exact Qt Widget classes. There's still nothing stopping others from reusing code from one widget category in another since all the functions are still defined in breezehelper.h and breezestyle.h.

Also, this change will make it pretty difficult to use git blame and git bisect in the future to track possible regressions. (to my knowledge at least). Might as well start a new repository.

ndavis added a comment.EditedNov 27 2019, 4:13 AM

Also (and somewhat independently from this patch): with Qt6/KF6 being around the corner, I somewhat wonder about the utility of rewritting and hacking breeze at that time. Would this make sense to actually leave breeze (which is a rather well-working theme, with very little bug reports) unchanged, and start working anew on a new widget style targeting KF6 ? Would that not be more exciting, bring more people in, trigger new ideas, give more freedom ? And be a more ambitious task all in all ?
One could of course start from breeze, and apply all the new ideas about how the code should be organized, how highlight should look, and checkboxes, without disturbance for something which has lived largely unchanged (and with not so many complains) for all the kf5 lifetime ...

My two cents.

That's a fair point. I have thought a little bit about trying to start my own widget style and it would be easier to patch my unreleased theme a piece at a time instead of trying to do it all at once to prevent inconsistency between releases.

I have also been thinking that we might want to do the proposed "Breeze theme evolution" in a new theme (perhaps even just forked from Breeze) so that people people who like Breeze can keep it, and we can wipe the slate clean and have more design freedom.

ndavis abandoned this revision.Nov 27 2019, 6:14 AM

I'll abandon this and see how starting my own theme goes.