D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R121:eb9c4c080427: Fully remove `Application Name` from 
Details panel (authored by sharvey, committed by bruns).

REPOSITORY
  R121 Policykit (Polkit) KDE Agent

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12498?vs=44184=44206

REVISION DETAIL
  https://phabricator.kde.org/D12498

AFFECTED FILES
  AuthDialog.cpp
  AuthDialog.h
  authdetails.ui

To: bruns, ngraham, davidedmundson, sharvey
Cc: davidedmundson, bruns, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  Thanks!

REPOSITORY
  R121 Policykit (Polkit) KDE Agent

BRANCH
  arcpatch-D12498

REVISION DETAIL
  https://phabricator.kde.org/D12498

To: bruns, ngraham, davidedmundson, sharvey
Cc: davidedmundson, bruns, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns updated this revision to Diff 44184.
bruns added a comment.


  Update UI file
  Split out "description" change

REPOSITORY
  R121 Policykit (Polkit) KDE Agent

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12498?vs=42375=44184

BRANCH
  arcpatch-D12498

REVISION DETAIL
  https://phabricator.kde.org/D12498

AFFECTED FILES
  AuthDialog.cpp
  AuthDialog.h
  authdetails.ui

To: bruns, ngraham, davidedmundson, sharvey
Cc: davidedmundson, bruns, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> AuthDialog.cpp:325
>  setupUi(this);
>  
> -// better N/A than a blank space

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

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


  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`

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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey added a comment.


  @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.

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


  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?

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added a comment.


  In 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.
  
  
  The diffstat for the file I attached is:
  9 changed lines
  26 removed lines
  6 added lines

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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey added a comment.


  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.

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


  In 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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey added a comment.


  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.

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added a comment.


  Try this one:
  F6290609: authdetails.ui 
  Also fixed up vertical alignment, should be the same for all labels.

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


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

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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey added inline comments.

INLINE COMMENTS

> bruns wrote in authdetails.ui:20
> 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.

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.

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> authdetails.ui:20
>
> -   
> -
> - 
> -  
> -   75
> -   true
> -  
> +   
> +

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.

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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey updated this revision to Diff 42375.
sharvey added a comment.


  - Changed PolKit description to `Description not provided`
  
  Was: "Missing"

REPOSITORY
  R121 Policykit (Polkit) KDE Agent

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12498?vs=33002=42375

BRANCH
  arcpatch-D12498

REVISION DETAIL
  https://phabricator.kde.org/D12498

AFFECTED FILES
  AuthDialog.cpp
  AuthDialog.h
  authdetails.ui

To: sharvey, bruns, ngraham, davidedmundson
Cc: davidedmundson, bruns, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


  It landed; you wanna finish this up now?

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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey added a comment.


  As soon as D12311: Align lock icon with bold message text; reduce overall 
size of dialog  is landed, I'll change this 
string.

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added a comment.


  In D12498#296546 , @ngraham wrote:
  
  > 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.

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


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

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added a comment.


  In D12498#296414 , @ngraham wrote:
  
  > ...  but I'd still prefer something a little bit more descriptive like "Not 
provided by "
  
  
  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:
  
  | Action  | Eject media from a system drive|
  | Action ID:  | org.freedesktop.udisks2.eject-media-system |
  | Vendor: | //The UDisks Project //|
  | 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 |
  |

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


  "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 "
  
  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?

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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey added a comment.


  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.

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


  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 ". That would convey the 
same point, but provide a hint that it's the app's fault, not ours.

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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey added a comment.


  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.

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added a comment.


  In D12498#278422 , @ngraham wrote:
  
  > 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).

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


  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.

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added a comment.


  In D12498#278380 , @ngraham wrote:
  
  > 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:  
 
  
  e.g.
  
Action:  'Description' not provided, please file a bug report
 org.foo.bar

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


  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.

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added a comment.


  In D12498#278285 , @ngraham wrote:
  
  > 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.

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


  In D12498#278282 , @bruns wrote:
  
  > In D12498#272379 , @ngraham 
wrote:
  >
  > > 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.

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added a comment.


  In D12498#272379 , @ngraham wrote:
  
  > 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.

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

D12498: Fully remove `Application Name` from Details panel

[Nathaniel Graham]

ngraham added a comment.


  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.

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added a comment.


  @sharvey can you address the remaining nitpicks?

INLINE COMMENTS

> sharvey wrote in AuthDialog.cpp:349
> 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.

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

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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey added inline comments.

INLINE COMMENTS

> bruns wrote in AuthDialog.cpp:349
> 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"

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.

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

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> authdetails.ui:26
>   
> -  Action:
> +  Action ID:
>   

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

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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey updated this revision to Diff 33002.
sharvey marked an inline comment as done.
sharvey added a comment.


  - Changed `Not Available` to `Missing`

REPOSITORY
  R121 Policykit (Polkit) KDE Agent

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12498?vs=33000=33002

BRANCH
  remove-appname (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12498

AFFECTED FILES
  AuthDialog.cpp
  AuthDialog.h
  authdetails.ui

To: sharvey, bruns, ngraham, davidedmundson
Cc: davidedmundson, bruns, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey updated this revision to Diff 33000.
sharvey added a comment.


  - Correct for use of `isEmpty()`; changed `Action ID` to simply `ID`

REPOSITORY
  R121 Policykit (Polkit) KDE Agent

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12498?vs=32993=33000

BRANCH
  remove-appname (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12498

AFFECTED FILES
  AuthDialog.cpp
  AuthDialog.h
  authdetails.ui

To: sharvey, bruns, ngraham, davidedmundson
Cc: davidedmundson, bruns, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12498: Fully remove `Application Name` from Details panel

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> AuthDialog.cpp:345
>  // neither isEmpty() or isNull() worked (?)
>  if (actionDescription.description() == "") {
>  QFont descrFont(action_label->font());

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

> AuthDialog.cpp:349
>  action_label->setFont(descrFont);
> -action_label->setText(i18n("Not Applicable"));
> +action_label->setText(i18n("Not Available"));
>  } else {

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
>   
> -  Action:
> +  Action ID:
>   

I think "ID" is sufficient

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

D12498: Fully remove `Application Name` from Details panel

[Scott Harvey]

sharvey created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
sharvey requested review of this revision.

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

TEST PLAN
  @bruns @ngraham @davidedmundson

REPOSITORY
  R121 Policykit (Polkit) KDE Agent

BRANCH
  remove-appname (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12498

AFFECTED FILES
  AuthDialog.cpp
  AuthDialog.h
  authdetails.ui

To: sharvey
Cc: davidedmundson, bruns, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart