D12498: Fully remove `Application Name` from Details panel

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://phabricat

D12498: Fully remove `Application Name` from Details panel

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: davi

D12498: Fully remove `Application Name` from Details panel

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&id=44184 BRANCH arcpatch-D12498 REVISION DETAIL https:

D12498: Fully remove `Application Name` from Details panel

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 Age

D12498: Fully remove `Application Name` from Details panel

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, d

D12498: Fully remove `Application Name` from Details panel

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 g

D12498: Fully remove `Application Name` from Details panel

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, davided

D12498: Fully remove `Application Name` from Details panel

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 ol

D12498: Fully remove `Application Name` from Details panel

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 c

D12498: Fully remove `Application Name` from Details panel

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

D12498: Fully remove `Application Name` from Details panel

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 X

D12498: Fully remove `Application Name` from Details panel

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

D12498: Fully remove `Application Name` from Details panel

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 c

D12498: Fully remove `Application Name` from Details panel

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

D12498: Fully remove `Application Name` from Details panel

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 re

D12498: Fully remove `Application Name` from Details panel

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&id=42375 BRANCH arcpat

D12498: Fully remove `Application Name` from Details panel

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, leslie

D12498: Fully remove `Application Name` from Details panel

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:

D12498: Fully remove `Application Name` from Details panel

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'

D12498: Fully remove `Application Name` from Details panel

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, plas

D12498: Fully remove `Application Name` from Details panel

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 de

D12498: Fully remove `Application Name` from Details panel

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,

D12498: Fully remove `Application Name` from Details panel

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

D12498: Fully remove `Application Name` from Details panel

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 pr

D12498: Fully remove `Application Name` from Details panel

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 REVISI

D12498: Fully remove `Application Name` from Details panel

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.

D12498: Fully remove `Application Name` from Details panel

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

D12498: Fully remove `Application Name` from Details panel

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

D12498: Fully remove `Application Name` from Details panel

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: sha

D12498: Fully remove `Application Name` from Details panel

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 ApSt

D12498: Fully remove `Application Name` from Details panel

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 miss

D12498: Fully remove `Application Name` from Details panel

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

D12498: Fully remove `Application Name` from Details panel

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 c

D12498: Fully remove `Application Name` from Details panel

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 so

D12498: Fully remove `Application Name` from Details panel

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 o

D12498: Fully remove `Application Name` from Details panel

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

D12498: Fully remove `Application Name` from Details panel

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&id=33002 BRANCH remo

D12498: Fully remove `Application Name` from Details panel

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&id=33000 BRANCH remove-appname (

D12498: Fully remove `Application Name` from Details panel

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:

D12498: Fully remove `Application Name` from Details panel

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