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.
Plasma | |
Breeze |
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 with oxygen-demo5 and kuiviewer untitled.ui
No Linters Available |
No Unit Test Coverage |
Buildable 19196 | |
Build 19214: arc lint + arc unit |
Please tell me if something about the structure does not make sense to you or if you think improvements could be made.
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.
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.
They use the BreezePrivate namespace, so it seemed appropriate.
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.
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.
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.