Re: Review Request 113158: Implement queueing directly in KDialogJobUiDelegate

[Kevin Ottens]

> On Oct. 14, 2013, 9:05 a.m., Kevin Ottens wrote:
> > I'm not sold on bastardizing QErrorMessage for that feature. The intent was 
> > more to have some code similar to what KDialogQueue (moved to KDE4Support) 
> > was doing directly in KDialogJobUiDelegate implementation.
> 
> David Faure wrote:
> I'm surprised by this. Yes our initial idea was doing it in KF5, but if 
> we can get what we need from Qt, isn't that even better?
> QErrorMessage has two features (queuing and dont-show-again checkbox), it 
> makes sense to me to be able to use one and not the other.
> 
> 
> The only reason I can see where this Qt patch wouldn't be enough, would 
> be if we need queueing of more complex dialogs than simple error 
> messageboxes. But IIRC the only use case we have are the error messageboxes 
> from KIO... right?

Yes, the only use case we have are the error message boxes from KIO.

I see your point regarding QErrorMessage, my concern is that the two features 
are not implemented in a completely orthogonal way in QErrorMessage and that 
the don't show again behavior could be pulled in. That's why I think we should 
have more control there and manage it ourselves. Still I can be wrong indeed.


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113158/#review41683
---


On Oct. 31, 2013, 10:15 a.m., Rohan Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113158/
> ---
> 
> (Updated Oct. 31, 2013, 10:15 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Instead of implementing queueing in KDialogJobUiDelegate, I'm making use of 
> QErrorMessage which has queueing built into it. We also inherit the "Show 
> this message again" checkbox, but I have a review request here 
> https://codereview.qt-project.org/67243 that adds new API to Qt 5.3 , so we 
> can hide that once 5.3 comes out ( if that makes sense ).
> 
> 
> Diffs
> -
> 
>   staging/kjobwidgets/src/kdialogjobuidelegate.h 5d17a4d 
>   staging/kjobwidgets/src/kdialogjobuidelegate.cpp 29c2bae 
> 
> Diff: http://git.reviewboard.kde.org/r/113158/diff/
> 
> 
> Testing
> ---
> 
> Tested by writing a application that uses KIO to fetch an invalid site url. 
> Dialog pops up just fine.
> 
> 
> Thanks,
> 
> Rohan Garg
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113158: Implement queueing directly in KDialogJobUiDelegate

[David Faure]

> On Oct. 14, 2013, 9:05 a.m., Kevin Ottens wrote:
> > I'm not sold on bastardizing QErrorMessage for that feature. The intent was 
> > more to have some code similar to what KDialogQueue (moved to KDE4Support) 
> > was doing directly in KDialogJobUiDelegate implementation.

I'm surprised by this. Yes our initial idea was doing it in KF5, but if we can 
get what we need from Qt, isn't that even better?
QErrorMessage has two features (queuing and dont-show-again checkbox), it makes 
sense to me to be able to use one and not the other.


The only reason I can see where this Qt patch wouldn't be enough, would be if 
we need queueing of more complex dialogs than simple error messageboxes. But 
IIRC the only use case we have are the error messageboxes from KIO... right?


- David


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113158/#review41683
---


On Oct. 31, 2013, 10:15 a.m., Rohan Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113158/
> ---
> 
> (Updated Oct. 31, 2013, 10:15 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Instead of implementing queueing in KDialogJobUiDelegate, I'm making use of 
> QErrorMessage which has queueing built into it. We also inherit the "Show 
> this message again" checkbox, but I have a review request here 
> https://codereview.qt-project.org/67243 that adds new API to Qt 5.3 , so we 
> can hide that once 5.3 comes out ( if that makes sense ).
> 
> 
> Diffs
> -
> 
>   staging/kjobwidgets/src/kdialogjobuidelegate.h 5d17a4d 
>   staging/kjobwidgets/src/kdialogjobuidelegate.cpp 29c2bae 
> 
> Diff: http://git.reviewboard.kde.org/r/113158/diff/
> 
> 
> Testing
> ---
> 
> Tested by writing a application that uses KIO to fetch an invalid site url. 
> Dialog pops up just fine.
> 
> 
> Thanks,
> 
> Rohan Garg
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113158: Implement queueing directly in KDialogJobUiDelegate

[Rohan Garg]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113158/
---

(Updated Oct. 31, 2013, 10:15 a.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks.


Repository: kdelibs


Description
---

Instead of implementing queueing in KDialogJobUiDelegate, I'm making use of 
QErrorMessage which has queueing built into it. We also inherit the "Show this 
message again" checkbox, but I have a review request here 
https://codereview.qt-project.org/67243 that adds new API to Qt 5.3 , so we can 
hide that once 5.3 comes out ( if that makes sense ).


Diffs
-

  staging/kjobwidgets/src/kdialogjobuidelegate.h 5d17a4d 
  staging/kjobwidgets/src/kdialogjobuidelegate.cpp 29c2bae 

Diff: http://git.reviewboard.kde.org/r/113158/diff/


Testing
---

Tested by writing a application that uses KIO to fetch an invalid site url. 
Dialog pops up just fine.


Thanks,

Rohan Garg

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113158: Implement queueing directly in KDialogJobUiDelegate

[Kevin Ottens]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113158/#review41683
---


I'm not sold on bastardizing QErrorMessage for that feature. The intent was 
more to have some code similar to what KDialogQueue (moved to KDE4Support) was 
doing directly in KDialogJobUiDelegate implementation.

- Kevin Ottens


On Oct. 7, 2013, 6:28 p.m., Rohan Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113158/
> ---
> 
> (Updated Oct. 7, 2013, 6:28 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Instead of implementing queueing in KDialogJobUiDelegate, I'm making use of 
> QErrorMessage which has queueing built into it. We also inherit the "Show 
> this message again" checkbox, but I have a review request here 
> https://codereview.qt-project.org/67243 that adds new API to Qt 5.3 , so we 
> can hide that once 5.3 comes out ( if that makes sense ).
> 
> 
> Diffs
> -
> 
>   staging/kjobwidgets/src/kdialogjobuidelegate.h 5d17a4d 
>   staging/kjobwidgets/src/kdialogjobuidelegate.cpp 29c2bae 
> 
> Diff: http://git.reviewboard.kde.org/r/113158/diff/
> 
> 
> Testing
> ---
> 
> Tested by writing a application that uses KIO to fetch an invalid site url. 
> Dialog pops up just fine.
> 
> 
> Thanks,
> 
> Rohan Garg
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 113158: Implement queueing directly in KDialogJobUiDelegate

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113158/#review41578
---


You should import the test app into kio/tests (and possibly mention it here in 
a comment)


staging/kjobwidgets/src/kdialogjobuidelegate.cpp


It should be created on demand.

(add a getter here that creates it if still null)


- David Faure


On Oct. 7, 2013, 6:28 p.m., Rohan Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113158/
> ---
> 
> (Updated Oct. 7, 2013, 6:28 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Instead of implementing queueing in KDialogJobUiDelegate, I'm making use of 
> QErrorMessage which has queueing built into it. We also inherit the "Show 
> this message again" checkbox, but I have a review request here 
> https://codereview.qt-project.org/67243 that adds new API to Qt 5.3 , so we 
> can hide that once 5.3 comes out ( if that makes sense ).
> 
> 
> Diffs
> -
> 
>   staging/kjobwidgets/src/kdialogjobuidelegate.h 5d17a4d 
>   staging/kjobwidgets/src/kdialogjobuidelegate.cpp 29c2bae 
> 
> Diff: http://git.reviewboard.kde.org/r/113158/diff/
> 
> 
> Testing
> ---
> 
> Tested by writing a application that uses KIO to fetch an invalid site url. 
> Dialog pops up just fine.
> 
> 
> Thanks,
> 
> Rohan Garg
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 113158: Implement queueing directly in KDialogJobUiDelegate

[Rohan Garg]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113158/
---

Review request for KDE Frameworks.


Repository: kdelibs


Description
---

Instead of implementing queueing in KDialogJobUiDelegate, I'm making use of 
QErrorMessage which has queueing built into it. We also inherit the "Show this 
message again" checkbox, but I have a review request here 
https://codereview.qt-project.org/67243 that adds new API to Qt 5.3 , so we can 
hide that once 5.3 comes out ( if that makes sense ).


Diffs
-

  staging/kjobwidgets/src/kdialogjobuidelegate.h 5d17a4d 
  staging/kjobwidgets/src/kdialogjobuidelegate.cpp 29c2bae 

Diff: http://git.reviewboard.kde.org/r/113158/diff/


Testing
---

Tested by writing a application that uses KIO to fetch an invalid site url. 
Dialog pops up just fine.


Thanks,

Rohan Garg

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel