Fully remove `Application Name` from Details panel
ClosedPublic

Authored by bruns on Apr 24 2018, 4:05 PM.

Details

Summary

Application Name removed from PolicyKit details panel (backend code removed long ago)

Addresses concern raised in R121:b340539eca13

Diff Detail

Repository
R121 Policykit (Polkit) KDE Agent
Branch
remove-appname (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
sharvey created this revision.Apr 24 2018, 4:05 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 24 2018, 4:05 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sharvey requested review of this revision.Apr 24 2018, 4:05 PM
sharvey edited the summary of this revision. (Show Details)Apr 24 2018, 4:09 PM
sharvey edited the test plan for this revision. (Show Details)
sharvey added reviewers: bruns, ngraham, davidedmundson.
bruns added inline comments.Apr 24 2018, 5:10 PM
AuthDialog.cpp
343

== "" should be identical to .isEmpty(), please recheck

347

According to https://www.freedesktop.org/software/polkit/docs/latest/polkit.8.html#polkit-rules
description is not optional, so the correct statement is "Missing"

authdetails.ui
26

I think "ID" is sufficient

sharvey updated this revision to Diff 33000.Apr 24 2018, 5:27 PM
  • Correct for use of isEmpty(); changed Action ID to simply ID
sharvey updated this revision to Diff 33002.Apr 24 2018, 5:29 PM
sharvey marked an inline comment as done.
  • Changed Not Available to Missing
sharvey marked 2 inline comments as done.Apr 24 2018, 5:31 PM
bruns requested changes to this revision.Apr 27 2018, 7:40 PM
bruns added inline comments.
authdetails.ui
26

Can you also change it for the placeholder label (row 2, col 1) - convention is to set both (actual label and placeholder) to the same value.

And while you are at it, use the same colspan value here as for the other col 1 entries (not you fault, missing since D11950).

This revision now requires changes to proceed.Apr 27 2018, 7:40 PM
sharvey added inline comments.May 5 2018, 12:54 AM
AuthDialog.cpp
347

I was checking to see if this was still open, and would like to discuss "Missing". I considered "Missing" while I was originally coding this, but thought it sounded a little negative, like we weren't able to access the information. I realize that "Missing" is the official response per the FDO spec, could we settle on something like "Not Provided"? After all, it's not our fault the information is missing. Just an idea.

bruns added a comment.Jun 2 2018, 12:46 AM

@sharvey can you address the remaining nitpicks?

AuthDialog.cpp
347

I prefer 'missing', to give upstream a 'hint' they are doing something wrong.

I feel like if we go with "missing", users will blame us and we'll get bug reports ("If it's missing, why can't you provide it? Duh!").

My preference, if it's possible, would be to simply omit any fields for which we don't have information available. No need to confuse users.

bruns added a comment.Jun 14 2018, 2:01 PM

I feel like if we go with "missing", users will blame us and we'll get bug reports ("If it's missing, why can't you provide it? Duh!").

Its simple "We can not provide it because upstream was to lazy to read and follow the spec. Please raise a upstream bug report". If we just omit it, we encourage upstream to keep shipping faulty polkit action files.

Btw, which action files are affected by this problem - I have not a single action file on my system which lacks a description tag.

I feel like if we go with "missing", users will blame us and we'll get bug reports ("If it's missing, why can't you provide it? Duh!").

Its simple "We can not provide it because upstream was to lazy to read and follow the spec. Please raise a upstream bug report". If we just omit it, we encourage upstream to keep shipping faulty polkit action files.

This concept (and the proposed label) is suitable for a developer audience, not a regular user audience.

We have the same issue in Discover when upstreams don't ship enough ApStream metadata. We omit anything that's missing instead of labeling the missing sections with some kind of admonishment to the upstreams. Developers like us should take care of browbeating non-compliant upstream software vendors; we shouldn't push that work onto users.

bruns added a comment.Jun 14 2018, 2:28 PM

This concept (and the proposed label) is suitable for a developer audience, not a regular user audience.

We have the same issue in Discover when upstreams don't ship enough ApStream metadata. We omit anything that's missing instead of labeling the missing sections with some kind of admonishment to the upstreams. Developers like us should take care of browbeating non-compliant upstream software vendors; we shouldn't push that work onto users.

The description is visible in the "Details" panel only, which is targeted at a Developer/Admin audience. So the "users should not be bothered" IMHO does not apply here. If you just omit it, nobody will report the problematic actions, and nobody will fill an upstream bug report.

Ah yeah, that makes sense. Instead of "Missing", how about "Not available; please file a bug with the developer of this software" ...Or something like that.

bruns added a comment.Jun 14 2018, 4:36 PM

Ah yeah, that makes sense. Instead of "Missing", how about "Not available; please file a bug with the developer of this software" ...Or something like that.

Somewhat to verbose, and it needs 'Description' somewhere in the sentence, as it looks like this:

Action:  <Description>
         <action-id>

e.g.

Action:  'Description' not provided, please file a bug report
         org.foo.bar

Looks good to me, though the string needs to make clear that the bug report should go to the 3rd-party app developer, not to us! Otherwise we'll get a bunch of un-actionable bugs.

bruns added a comment.Jun 14 2018, 5:03 PM

Looks good to me, though the string needs to make clear that the bug report should go to the 3rd-party app developer, not to us! Otherwise we'll get a bunch of un-actionable bugs.

I think its sufficiently clear. Directly below is the action id, which, as in reverse URL format, is a good indicator whom to address (mind the target audience again). Also there hopefully is the "Vendor"/"Vendor URL" tag. There may be a few cases where a BR ends up in the KDE bugzilla, but I think we can deal with these (either file a bug report, or point the reporter to upstream).

I'll get back to work on this. I've got a current open patch I need to finish up first, then I'll resume work on this. Should be a couple days at most.

Thanks for all the debate and decision on the messaging.

I'm not comfortable with the string "Missing". It's a developer-centric string that's not user-friendly, and it doesn't help the user figure out what's wrong or whose fault it might be. We'd get bug reports over this; people would say, "If it's missing, KDE should provide it!!!" And we'd reply, "It's missing from the app, not from us," and this would happen a dozen times.

How about instead, "Not provided by <application>". That would convey the same point, but provide a hint that it's the app's fault, not ours.

I'm still in favor of the brief but descriptive "Not Provided". This is an advanced developer field. I could add a tooltip with some additional information. My point is to convey the fact that "the vendor is supposed to provide this information, but did not."

To me, that makes clear that we didn't "lose" the data or that there's some bug preventing it from being shown.

"Not Provided" makes it clear that someone didn't provide the information, but who? If we can't agree on anything else, I'll agree to compromise on "Not Provided", but I'd still prefer something a little bit more descriptive like "Not provided by <application>"

I mean, if you show the detailed information section in the first place, it's a good bet that you want to see all the information, right? Why be coy or hide relevant information behind a tooltip? Why make the user guess?

bruns added a comment.Jul 23 2018, 5:19 PM

... but I'd still prefer something a little bit more descriptive like "Not provided by <application>"

The problem is you can not really know the application. While often the Action-ID is derived from the upstream URL in reverse notation, in general it is independent from a specific implementation/application.

After removing the dreadful "Application: ..." line, the Details pane looks like this:

ActionEject media from a system drive
Action ID:org.freedesktop.udisks2.eject-media-system
Vendor:The UDisks Project <link to github>
polkit.subject-pid:6457
polkit.caller-pid:6476

"Vendor" is optional according to the spec. So if we had a malformed rule file, it may read like this:

Action'Description' not provided, please file a bug report
Action ID:org.freedesktop.udisks2.eject-media-system
polkit.subject-pid:6457
polkit.caller-pid:6476

Darn, that's a shame. "Not Provided" it is, then. @bruns, are you okay with this?

Darn, that's a shame. "Not Provided" it is, then. @bruns, are you okay with this?

Either

'Description' not provided, please file a bug report

or

'Description' not provided

is fine for me. It should contain the 'Description' keyword, though. If you dislike the quotes, italics is also fine.

It landed; you wanna finish this up now?

sharvey updated this revision to Diff 42375.Sep 26 2018, 3:22 PM
  • Changed PolKit description to Description not provided

Was: "Missing"

bruns added inline comments.Sep 27 2018, 10:52 PM
authdetails.ui
20–23

Can you try to restore/use the same item order in the XML file as previously?
Should make the diff significantly smaller, and easier to review.

sharvey added inline comments.Sep 27 2018, 11:03 PM
authdetails.ui
20–23

The XML file is auto-generated by the form editor in QtCreator. I don’t have any control over it. And I don’t want to mess with it by hand - too easy to break.

The XML file is auto-generated by the form editor in QtCreator. I don’t have any control over it. And I don’t want to mess with it by hand - too easy to break.

You pretty much have to, or else the diff is impossible to review, and it's easy for unintentional changes to sneak in. .ui files just aren't any fun, and this issue is why I suggested porting it to QML in T8569.

Try this one:


Also fixed up vertical alignment, should be the same for all labels.

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.

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.

Truthfuly, 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.

I've got an email chain of 48 messages for a simple string change. And I expect it's only going to grow. This has been nitpicked to death, and I'm not sure why. We've beaten this dead horse to a pulp.

@bruns - If you know what you want done with the XML file, I'll gladly let you do it. I simply don't get it.

@ngraham - You know I'm not combative or confrontational by nature. But this is beyond my understanding.

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. :)

This was copied from D12311: Align lock icon with bold message text; reduce overall size of dialog, which languished unapproved and un-landed for a long time. Indeed, I should have made this a separate patch, but I'd only been a contributor for about 2-3 months at this time. It duplicates a lot of what's in D12311, which is now finally closed.

I propose scrapping this revision and starting it from scratch, based off D12311. I don't think I have the knowledge of the PolicyKit backend to rewrite it in QML. And it'll likely need a C++ component as well; combining the two is a topic I'm still coming to terms with.

I apologize for my frustration. It rarely gets to me, but things in the Real World are very stressful at the moment.

bruns added a comment.Sep 28 2018, 9:47 PM

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.

The diffstat for the file I attached is:
9 changed lines
26 removed lines
6 added lines

It's okay, we've all been there. :)

Can you maybe take @bruns' .ui file for this patch and then we can re-commence review?

@ngraham : Now that I understand what went wrong here, I really do think this particular diff be abandoned. It mistakenly contains unrelated material from a different diff (D12311) - which we probably shouldn't re-submit. I will get a fresh master, which will have D12311 already incorporated and make @bruns 's small change. That should resolve the issue without any drama.

My weekend is booked solid, but I'll try to squeeze it in.

No problem, glad we've got a path forward!

FWIW, you can re-base this on the current master with git pull --rebase origin master

sharvey marked 3 inline comments as done.Oct 1 2018, 4:42 PM
bruns added inline comments.Oct 1 2018, 6:13 PM
AuthDialog.cpp
325

This hunk should go into D15885, also all the changes with appname/m_appname

bruns commandeered this revision.Oct 24 2018, 8:32 PM
bruns edited reviewers, added: sharvey; removed: bruns.
bruns updated this revision to Diff 44184.Oct 24 2018, 8:36 PM

Update UI file
Split out "description" change

ngraham accepted this revision.Oct 25 2018, 4:36 AM

Thanks!

This revision is now accepted and ready to land.Oct 25 2018, 4:36 AM
This revision was automatically updated to reflect the committed changes.