D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread Friedrich W. H. Kossebau 
kossebau added a comment.


  (nullptr used as arguments always make me consider finally working on a patch 
to add to normal kdevelop the feature "Show argument names at call site" like 
demoed here among other things:  
https://kate-editor.org/wp-content/uploads/2018/08/inline-note-anim.gif so far 
hoped someone else would do it, but seems it is time to help myself possibly)

REPOSITORY
  R288 KJobWidgets

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread David Faure 
dfaure closed this revision.

REPOSITORY
  R288 KJobWidgets

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread Kevin Ottens 
ervin added a comment.


  Agreed, nullptr is going to be the boolean flag of our time, before it was 0 
though, so still an improvement. ;-)
  
  More seriously, here I'm not sure how to avoid it, at least it's a case of 
"if you feel like passing nullptr here you might be doing something wrong".

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread Friedrich W. H. Kossebau 
kossebau added a comment.


  Not into details, so if there is no sane default, forcing developers to pass 
something sane here  make sense
  Second thought I also had was avoiding code which uses `nullptr` as 
arguments, which harms humans reading code a bit as in, "nullptr of what?!!" 
(compare boolean flags) ;)

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread Kevin Ottens 
ervin accepted this revision.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread Kevin Ottens 
ervin added a comment.


  In D28742#646050 , @dfaure wrote:
  
  > In D28742#646009 , @kossebau 
wrote:
  >
  > > And perhaps could be defaulted to nullptr, for use-cases which do not 
have a window at hand and are fine with any default?
  >
  >
  > I've been wondering. But people tend to forget to do so, and in most cases, 
if we choose the dialog delegate, then there's a QWidget based window somewhere.
  >  Plasma uses KNotificationJobUiDelegate so it's not a problem here.
  >  My thinking is that I'd rather force people to think about it, and 
possibly pass a nullptr in case there's really no window around.
  
  
  +1, I don't think defaulting to nullptr is a good idea.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread David Faure 
dfaure added a comment.


  In D28742#646009 , @kossebau wrote:
  
  > `window` parameter wants API dox mentioning, though.
  
  
  Oops, I thought I did that. Fixed.
  
  > And perhaps could be defaulted to nullptr, for use-cases which do not have 
a window at hand and are fine with any default?
  
  I've been wondering. But people tend to forget to do so, and in most cases, 
if we choose the dialog delegate, then there's a QWidget based window somewhere.
  Plasma uses KNotificationJobUiDelegate so it's not a problem here.
  My thinking is that I'd rather force people to think about it, and possibly 
pass a nullptr in case there's really no window around.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread David Faure 
dfaure updated this revision to Diff 79857.
dfaure added a comment.


  Expand docs about the associated window

REPOSITORY
  R288 KJobWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28742?vs=79848=79857

BRANCH
  master

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

AFFECTED FILES
  src/kdialogjobuidelegate.cpp
  src/kdialogjobuidelegate.h

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread Friedrich W. H. Kossebau 
kossebau added a comment.


  `window` parameter wants API dox mentioning, though. And perhaps could be 
defaulted to nullptr, for use-cases which do not have a window at hand and are 
fine with any default?

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread Kevin Ottens 
ervin accepted this revision.
ervin added a comment.


  Indeed, good point.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread David Faure 
dfaure updated this revision to Diff 79848.
dfaure added a comment.


  Add QWidget *window parameter. Even better, no?
  
  Needed for dialog boxes to respect stacking order, centering to parent, focus 
going back to parent after closing...

REPOSITORY
  R288 KJobWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28742?vs=79840=79848

BRANCH
  master

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

AFFECTED FILES
  src/kdialogjobuidelegate.cpp
  src/kdialogjobuidelegate.h

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread Kevin Ottens 
ervin accepted this revision.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread David Faure 
dfaure updated this revision to Diff 79840.
dfaure marked an inline comment as done.
dfaure added a comment.


  explicit

REPOSITORY
  R288 KJobWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28742?vs=79820=79840

BRANCH
  master

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

AFFECTED FILES
  src/kdialogjobuidelegate.cpp
  src/kdialogjobuidelegate.h

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread David Faure 
dfaure marked an inline comment as done.
dfaure added inline comments.

INLINE COMMENTS

> kossebau wrote in kdialogjobuidelegate.h:51
> Why no explicit?

good point

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread Friedrich W. H. Kossebau 
kossebau added inline comments.

INLINE COMMENTS

> kdialogjobuidelegate.h:51
> + */
> +KDialogJobUiDelegate(KJobUiDelegate::Flags flags); // KF6 TODO merge 
> with default constructor, using AutoHandlingDisabled as default value
> +

Why no explicit?

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread Kevin Ottens 
ervin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

To: dfaure, broulik, davidedmundson, ervin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28742: Add KDialogJobUiDelegate(KJobUiDelegate::Flags) constructor

2020-04-11 Thread David Faure 
dfaure created this revision.
dfaure added reviewers: broulik, davidedmundson, ervin.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  Requires: D28741 

TEST PLAN
  Ported kruntest to ApplicationLauncherJob, used this constructor, works.

REPOSITORY
  R288 KJobWidgets

BRANCH
  master

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

AFFECTED FILES
  src/kdialogjobuidelegate.cpp
  src/kdialogjobuidelegate.h

To: dfaure, broulik, davidedmundson, ervin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns