ngraham added a comment.
In D12498#333627 <https://phabricator.kde.org/D12498#333627>, @sharvey wrote: > I believe what you're perceiving in the XML file is in fact the result of a lot of changes made to the layout. Unneeded columns, rows, and spacers were deleted, causing "gaps" in the old XML file. In places, I added options like column spans and justifications. The XML file changed a lot. Right, and wouldn't those changes be unrelated to this patch? If it's just a simple string removal, why do we need so many layout changes? The layout changes may be useful (I believe they probably are) but shouldn't they be done in a separate patch? > In my opinion - which you can take or leave - I don't believe these XML files were intended to be human-read. The UI they generate is where the judgement and checking should come from, not the convoluted XML generated by QtCreator. I've seen diffs much larger than this one. That's their inherent problem, and I'm guessing one of the reasons why the Qt folks developed QML. It's a problem that has plagued machine-generated code files since forever. The reason why using Qt Creator to modify these files after the fact is a problem is because makes the, well, code review part of code review nearly impossible. The diff is just noise because so much has changed. It becomes very challenging to actually review because it's not apparent what's intentional, what might have snuck in by accident, or what's simply Qt Creator moving things around with no change in functionality. Yes, we can evaluate the UI that it produces to make sure that it's good. But UI review and code review are different for a good reason. A UI that looks and feels okay when you test it can still have hidden problems that are only revealed by reading the code. All changes to `.ui` files that I've ever made have been by hand, and I've seen other KDE contributors do the same. Yes, this does indeed suck. It's why I'm recommending either: - Redoing the changes to the `.ui` file by hand to keep the diff manageable and reviewable - First in another patch porting the `.ui` file to QML so that changes made by hand aren't torture > Truthfully, I'm not clear on what you expect me to do. The UI works fine. Monkeying with the XML to make it "pretty" seems a bit pointless. Take one of the above two options, or hand it over to @bruns, I guess. It's all good though. :) REPOSITORY R121 Policykit (Polkit) KDE Agent REVISION DETAIL https://phabricator.kde.org/D12498 To: sharvey, bruns, ngraham, davidedmundson Cc: davidedmundson, bruns, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
