Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Christoph Feck]

> On Oct. 12, 2012, 2:11 a.m., Commit Hook wrote:
> > This review has been submitted with commit 
> > a78c6a50dcb33028eb572bc260bdaca8f30a597a by Dawit Alemayehu to branch 
> > KDE/4.9.
> 
> Dawit Alemayehu wrote:
> Ahhh... I did not do this! How did this patch end up being cherry-picked 
> into 4.9 branch ??!?! It is only supposed to be in master for 4.10 release.

See bug 260282 comment #3.


- Christoph


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


On Oct. 8, 2012, 5:43 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 8, 2012, 5:43 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/760/
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/759/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

> On Oct. 12, 2012, 2:11 a.m., Commit Hook wrote:
> > This review has been submitted with commit 
> > a78c6a50dcb33028eb572bc260bdaca8f30a597a by Dawit Alemayehu to branch 
> > KDE/4.9.

Ahhh... I did not do this! How did this patch end up being cherry-picked into 
4.9 branch ??!?! It is only supposed to be in master for 4.10 release.


- Dawit


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


On Oct. 8, 2012, 5:43 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 8, 2012, 5:43 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/760/
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/759/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Commit Hook]

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


This review has been submitted with commit 
a78c6a50dcb33028eb572bc260bdaca8f30a597a by Dawit Alemayehu to branch KDE/4.9.

- Commit Hook


On Oct. 8, 2012, 5:43 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 8, 2012, 5:43 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/760/
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/759/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Oct. 8, 2012, 5:43 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 8, 2012, 5:43 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/759/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/760/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[David Faure]

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



konqueror/src/konqsessionmanager.cpp


Ah yes I misread, there's a call to icon.pixmap() in there. Sorry.


- David Faure


On Oct. 8, 2012, 5:43 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 8, 2012, 5:43 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/759/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/760/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

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

(Updated Oct. 8, 2012, 5:43 p.m.)


Review request for KDE Base Apps and David Faure.


Changes
---

More changes based on feedback. Also updated the comparison screenshots.


Description
---

The attached patch fixes one of those pet peeve bugs that infurate me from time 
to time by allowing me to unselect the sessions I do not want to be restored 
when Konqueror's restore session dialog pops up.


This addresses bug 260282.
http://bugs.kde.org/show_bug.cgi?id=260282


Diffs (updated)
-

  konqueror/src/konqsessionmanager.h ee629e4 
  konqueror/src/konqsessionmanager.cpp 68a003f 

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


Testing
---

* Unselected sessions should not be restored.
* If all available sessions are selected (the default), the behavior should 
remain the same as it is today.
* If all available sessions are unselected, disable the "Restore Session" 
button.


Screenshots (updated)
---

old restore dialog
  http://git.reviewboard.kde.org/r/106503/s/759/
new restore dialog
  http://git.reviewboard.kde.org/r/106503/s/760/


Thanks,

Dawit Alemayehu

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

> On Oct. 8, 2012, 10:30 a.m., David Faure wrote:
> > konqueror/src/konqsessionmanager.cpp, line 123
> > 
> >
> > Did you change this if() wrongly?
> > 
> > Now the code says "if I could find the dialog-warning icon, then ignore 
> > it and ask QStyle instead".
> > 
> > This makes no sense ;)

I did not change anything. That is exactly how it is in 
KMessageBox::createKMessageBox. Also I do not see how you can read the if 
statement the way you did. To me the if statement reads, "if you find the 
dialog-warning icon, then use the metrics from the current QStyle when setting 
it". 


> On Oct. 8, 2012, 10:30 a.m., David Faure wrote:
> > konqueror/src/konqsessionmanager.cpp, line 145
> > 
> >
> > All this generic code could be simplified for the use case at hand. 
> > Maybe after inserting a '\n' in the string (between the two sentences), and 
> > then all that squeezing and wrapping can be removed. Let's keep this simple 
> > and maintainable.

No need to add '\n' and hard code the line break. Simply setting the label 
wrapable and removing all the wrapping and squeezing related code is sufficient.


- Dawit


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


On Oct. 4, 2012, 3:50 a.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 4, 2012, 3:50 a.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> new restore dialog v3
>   http://git.reviewboard.kde.org/r/106503/s/750/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[David Faure]

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



konqueror/src/konqsessionmanager.cpp


Did you change this if() wrongly?

Now the code says "if I could find the dialog-warning icon, then ignore it 
and ask QStyle instead".

This makes no sense ;)



konqueror/src/konqsessionmanager.cpp


All this generic code could be simplified for the use case at hand. Maybe 
after inserting a '\n' in the string (between the two sentences), and then all 
that squeezing and wrapping can be removed. Let's keep this simple and 
maintainable.



konqueror/src/konqsessionmanager.cpp


This line and the one below, can be removed now. They don't do anything.



konqueror/src/konqsessionmanager.cpp


The label is two lines at most, it will never be bigger than a third of the 
desktop height. Remove usingScrollArea and the associated code.

The tree widget can be tall, yes, but not the label on top.


- David Faure


On Oct. 4, 2012, 3:50 a.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 4, 2012, 3:50 a.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> new restore dialog v3
>   http://git.reviewboard.kde.org/r/106503/s/750/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

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

(Updated Oct. 4, 2012, 3:50 a.m.)


Review request for KDE Base Apps and David Faure.


Changes
---

- Fixed compilation problem with previous patch.
- Reused code to set the windows group.


Description
---

The attached patch fixes one of those pet peeve bugs that infurate me from time 
to time by allowing me to unselect the sessions I do not want to be restored 
when Konqueror's restore session dialog pops up.


This addresses bug 260282.
http://bugs.kde.org/show_bug.cgi?id=260282


Diffs (updated)
-

  konqueror/src/konqsessionmanager.h ee629e4 
  konqueror/src/konqsessionmanager.cpp 68a003f 

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


Testing
---

* Unselected sessions should not be restored.
* If all available sessions are selected (the default), the behavior should 
remain the same as it is today.
* If all available sessions are unselected, disable the "Restore Session" 
button.


Screenshots
---

old restore dialog
  http://git.reviewboard.kde.org/r/106503/s/729/
new restore dialog
  http://git.reviewboard.kde.org/r/106503/s/731/
new restore dialog v2
  http://git.reviewboard.kde.org/r/106503/s/739/
new restore dialog v3
  http://git.reviewboard.kde.org/r/106503/s/750/


Thanks,

Dawit Alemayehu

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Thomas Lübking]

> On Oct. 2, 2012, 8:57 a.m., David Faure wrote:
> > konqueror/src/konqsessionmanager.cpp, line 166
> > 
> >
> > Does this really make any difference? Widgets are transparent by 
> > default, in Qt4...
> 
> Thomas Lübking wrote:
> To be more aggressive: DO NOT DO THAT! NEVER!
> 
> Qt::transparent is Qt::black with zero alpha component, so whenever 
> someone (UI Styles) test the the background color, they will get rubbish for 
> this widget (unless you check the alpha component and conditionally treat 
> this as invisible)
> 
> This approach showed up someone in the early KDE4 days and is since then 
> the pestilence in visual KDE code (so eg. bespin during polishment tests 
> widgets for doing this and tries to fix them up as good as possible)
> 
> use ::setAutoFillBackground(false) on ::viewport() but please DO NOT 
> FREAK THE PALETTE, seriously ;-)
> 
> Another issue in this regard are several QItemViews, because they operate 
> on hardcoded QPalette::Text, so if you want to make such view "transparent" 
> you should not only viewport()->setAutoFillBackground(false) but also (and 
> actually) will unfortunately have to "fix" the palette by aligning 
> palette().color(QPalette::Text) and 
> parentWidget().palette().color(QPalette::Window) - that includes filtering 
> QEvent::Application/PaletteChange and re-fix the colors under this incident 
> (smart approach is to check whether the color is already correct and not 
> trigger another PaletteChange event in that case ;-)
> 
> For the very same manner, please do not abuse setStyleSheet() for being 
> lazy to correct colors (now actually changing the palette, maybe even better: 
> set[Back|Fore]groundRole()) - you'll end up with hardcoded colors (even if 
> you select them at runtime) bypassing the style, and in 90% of all cases will 
> break bright on dark palettes.
> 
> Dawit Alemayehu wrote:
> For the record that piece of code was copied verbatim from 
> KMessageBox::createKMessageBox. If it is the wrong thing to do, then it needs 
> to be fixed there as well.

I'm not surprised.
It happens so often and happens so often "wrong", that there should ideally be 
a (static, but we'll need an eventfilter) helper function to do that (like 
static KStyle::makeTransparent(QAbstractScrollArea*), but i'm unsure about the 
"proper" class/namespace) in frameworks or so - and how to make ppl. use it.


- Thomas


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


On Oct. 2, 2012, 9:07 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 2, 2012, 9:07 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> new restore dialog v3
>   http://git.reviewboard.kde.org/r/106503/s/750/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

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

(Updated Oct. 2, 2012, 9:07 p.m.)


Review request for KDE Base Apps and David Faure.


Changes
---

- Updated patch based on feedback.
- Added code to check/uncheck the parent item (Window 1...), when all the 
session items are checked or unchecked.


Description
---

The attached patch fixes one of those pet peeve bugs that infurate me from time 
to time by allowing me to unselect the sessions I do not want to be restored 
when Konqueror's restore session dialog pops up.


This addresses bug 260282.
http://bugs.kde.org/show_bug.cgi?id=260282


Diffs (updated)
-

  konqueror/src/konqsessionmanager.h ee629e4 
  konqueror/src/konqsessionmanager.cpp 68a003f 

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


Testing
---

* Unselected sessions should not be restored.
* If all available sessions are selected (the default), the behavior should 
remain the same as it is today.
* If all available sessions are unselected, disable the "Restore Session" 
button.


Screenshots
---

old restore dialog
  http://git.reviewboard.kde.org/r/106503/s/729/
new restore dialog
  http://git.reviewboard.kde.org/r/106503/s/731/
new restore dialog v2
  http://git.reviewboard.kde.org/r/106503/s/739/
new restore dialog v3
  http://git.reviewboard.kde.org/r/106503/s/750/


Thanks,

Dawit Alemayehu

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

> On Oct. 2, 2012, 8:57 a.m., David Faure wrote:
> > konqueror/src/konqsessionmanager.cpp, line 257
> > 
> >
> > in the... ? :-)
> > 
> > (unfinished sentence)
> > 
> > It's easier to just move the connect to the block that creates the 
> > widget, anyway. No if() needed, then.

That was done on purpose. If we connect to the itemChanged signal when we 
create the tree widget, then everytime we call 
QTreeWiegdtItem::setCheckState(...), the signal will fire and we do not want 
that. I have updated the comment to reflect just that and I have moved the 
connection right below the tree widget creation.


- Dawit


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


On Oct. 1, 2012, 8:56 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 1, 2012, 8:56 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
>   konqueror/src/konqsessionmanager.h ee629e4 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> new restore dialog v3
>   http://git.reviewboard.kde.org/r/106503/s/750/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

> On Oct. 2, 2012, 8:57 a.m., David Faure wrote:
> > konqueror/src/konqsessionmanager.cpp, line 166
> > 
> >
> > Does this really make any difference? Widgets are transparent by 
> > default, in Qt4...
> 
> Thomas Lübking wrote:
> To be more aggressive: DO NOT DO THAT! NEVER!
> 
> Qt::transparent is Qt::black with zero alpha component, so whenever 
> someone (UI Styles) test the the background color, they will get rubbish for 
> this widget (unless you check the alpha component and conditionally treat 
> this as invisible)
> 
> This approach showed up someone in the early KDE4 days and is since then 
> the pestilence in visual KDE code (so eg. bespin during polishment tests 
> widgets for doing this and tries to fix them up as good as possible)
> 
> use ::setAutoFillBackground(false) on ::viewport() but please DO NOT 
> FREAK THE PALETTE, seriously ;-)
> 
> Another issue in this regard are several QItemViews, because they operate 
> on hardcoded QPalette::Text, so if you want to make such view "transparent" 
> you should not only viewport()->setAutoFillBackground(false) but also (and 
> actually) will unfortunately have to "fix" the palette by aligning 
> palette().color(QPalette::Text) and 
> parentWidget().palette().color(QPalette::Window) - that includes filtering 
> QEvent::Application/PaletteChange and re-fix the colors under this incident 
> (smart approach is to check whether the color is already correct and not 
> trigger another PaletteChange event in that case ;-)
> 
> For the very same manner, please do not abuse setStyleSheet() for being 
> lazy to correct colors (now actually changing the palette, maybe even better: 
> set[Back|Fore]groundRole()) - you'll end up with hardcoded colors (even if 
> you select them at runtime) bypassing the style, and in 90% of all cases will 
> break bright on dark palettes.

For the record that piece of code was copied verbatim from 
KMessageBox::createKMessageBox. If it is the wrong thing to do, then it needs 
to be fixed there as well.


- Dawit


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


On Oct. 1, 2012, 8:56 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 1, 2012, 8:56 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
>   konqueror/src/konqsessionmanager.h ee629e4 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> new restore dialog v3
>   http://git.reviewboard.kde.org/r/106503/s/750/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Thomas Lübking]

> On Oct. 2, 2012, 8:57 a.m., David Faure wrote:
> > konqueror/src/konqsessionmanager.cpp, line 166
> > 
> >
> > Does this really make any difference? Widgets are transparent by 
> > default, in Qt4...

To be more aggressive: DO NOT DO THAT! NEVER!

Qt::transparent is Qt::black with zero alpha component, so whenever someone (UI 
Styles) test the the background color, they will get rubbish for this widget 
(unless you check the alpha component and conditionally treat this as invisible)

This approach showed up someone in the early KDE4 days and is since then the 
pestilence in visual KDE code (so eg. bespin during polishment tests widgets 
for doing this and tries to fix them up as good as possible)

use ::setAutoFillBackground(false) on ::viewport() but please DO NOT FREAK THE 
PALETTE, seriously ;-)

Another issue in this regard are several QItemViews, because they operate on 
hardcoded QPalette::Text, so if you want to make such view "transparent" you 
should not only viewport()->setAutoFillBackground(false) but also (and 
actually) will unfortunately have to "fix" the palette by aligning 
palette().color(QPalette::Text) and 
parentWidget().palette().color(QPalette::Window) - that includes filtering 
QEvent::Application/PaletteChange and re-fix the colors under this incident 
(smart approach is to check whether the color is already correct and not 
trigger another PaletteChange event in that case ;-)

For the very same manner, please do not abuse setStyleSheet() for being lazy to 
correct colors (now actually changing the palette, maybe even better: 
set[Back|Fore]groundRole()) - you'll end up with hardcoded colors (even if you 
select them at runtime) bypassing the style, and in 90% of all cases will break 
bright on dark palettes.


- Thomas


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


On Oct. 1, 2012, 8:56 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 1, 2012, 8:56 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
>   konqueror/src/konqsessionmanager.h ee629e4 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> new restore dialog v3
>   http://git.reviewboard.kde.org/r/106503/s/750/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[David Faure]

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



konqueror/src/konqsessionmanager.h


Remove the WFlags stuff, it's a Qt3 thing that is not even used in this 
code anyway (I know, KDialog still has it, but that's for compat reasons).



konqueror/src/konqsessionmanager.cpp


Does this really make any difference? Widgets are transparent by default, 
in Qt4...



konqueror/src/konqsessionmanager.cpp


Another approach (slightly faster, and with less risks of memory leaks) 
would be to create windowItem on demand, inside the inner loop, and do
if (windowItem) {...}
here.



konqueror/src/konqsessionmanager.cpp


checked(bool) is slightly easier to handle than stateChanged(int), as long 
as you don't use tristate checkboxes.



konqueror/src/konqsessionmanager.cpp


This code knows what defaultCode is set to ... it was set to KDialog::Yes 
earlier. So this can be simplified to
setButtonFocus(KDialog::Yes), probably just after the setDefaultButton line.



konqueror/src/konqsessionmanager.cpp


in the... ? :-)

(unfinished sentence)

It's easier to just move the connect to the block that creates the widget, 
anyway. No if() needed, then.


- David Faure


On Oct. 1, 2012, 8:56 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 1, 2012, 8:56 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
>   konqueror/src/konqsessionmanager.h ee629e4 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> new restore dialog v3
>   http://git.reviewboard.kde.org/r/106503/s/750/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[David Faure]

> On Sept. 29, 2012, 9:20 a.m., David Faure wrote:
> > konqueror/src/konqsessionmanager.cpp, line 450
> > 
> >
> > Doesn't this lose ItemIsSelectable? I guess item->flags() | 
> > Qt::ItemIsUserCheckable would be better.
> 
> Dawit Alemayehu wrote:
> I did this on purpose. I do not want the items to be selectable. I just 
> want them to be checkable.

Ah, OK. No problem then -- assuming one can still use the keyboard to navigate 
this dialog.
[which doesn't require selection, but at least current-index navigation]


- David


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


On Oct. 1, 2012, 8:56 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Oct. 1, 2012, 8:56 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
>   konqueror/src/konqsessionmanager.h ee629e4 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> new restore dialog v3
>   http://git.reviewboard.kde.org/r/106503/s/750/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

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

(Updated Oct. 1, 2012, 8:56 p.m.)


Review request for KDE Base Apps and David Faure.


Changes
---

- Changed the code that shows the session dialog into its own KDialog instance.
- Changed the top level entry item to be checkable so that it can be checked or 
unchecked when the user select or unselect all the session entries underneath 
it.
- Updated the rest of the code accordingly.


Description
---

The attached patch fixes one of those pet peeve bugs that infurate me from time 
to time by allowing me to unselect the sessions I do not want to be restored 
when Konqueror's restore session dialog pops up.


This addresses bug 260282.
http://bugs.kde.org/show_bug.cgi?id=260282


Diffs (updated)
-

  konqueror/src/konqsessionmanager.cpp 68a003f 
  konqueror/src/konqsessionmanager.h ee629e4 

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


Testing
---

* Unselected sessions should not be restored.
* If all available sessions are selected (the default), the behavior should 
remain the same as it is today.
* If all available sessions are unselected, disable the "Restore Session" 
button.


Screenshots (updated)
---

old restore dialog
  http://git.reviewboard.kde.org/r/106503/s/729/
new restore dialog
  http://git.reviewboard.kde.org/r/106503/s/731/
new restore dialog v2
  http://git.reviewboard.kde.org/r/106503/s/739/
new restore dialog v3
  http://git.reviewboard.kde.org/r/106503/s/750/


Thanks,

Dawit Alemayehu

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

> On Sept. 29, 2012, 9:36 a.m., Pino Toscano wrote:
> > konqueror/src/konqsessionmanager.cpp, lines 343-345
> > 
> >
> > i18n: what do these "filter-..." contexts mean?

I copied those straight out of KMessageBox's code. Since I do not really know 
what they mean, I will simply remove it and use "yes", "no" and "cancel" 
instead.


- Dawit


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


On Sept. 28, 2012, 4:45 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Sept. 28, 2012, 4:45 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

> On Sept. 29, 2012, 9:20 a.m., David Faure wrote:
> > konqueror/src/konqsessionmanager.cpp, line 450
> > 
> >
> > Doesn't this lose ItemIsSelectable? I guess item->flags() | 
> > Qt::ItemIsUserCheckable would be better.

I did this on purpose. I do not want the items to be selectable. I just want 
them to be checkable.


> On Sept. 29, 2012, 9:20 a.m., David Faure wrote:
> > konqueror/src/konqsessionmanager.cpp, line 339
> > 
> >
> > This code would be better with a KDialog subclass, rather than with all 
> > the code in a function -- and then you need a slot in the calling class, 
> > and an ugly qobject_cast...
> > 
> > If you split this up into a proper class, then it can have its own 
> > slots and member variables, and this will increase modularity and 
> > reusability. (and readability).
> > 
> > Thanks.

Good point. I will redo it that way then.


- Dawit


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


On Sept. 28, 2012, 4:45 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Sept. 28, 2012, 4:45 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Pino Toscano]

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



konqueror/src/konqsessionmanager.cpp


i18n: what do these "filter-..." contexts mean?



konqueror/src/konqsessionmanager.cpp


KIcon icon("dialog-warning");
is enough, pixmap() should load the correct icon.



konqueror/src/konqsessionmanager.cpp


i18n: remove the space before the question mark, please


- Pino Toscano


On Sept. 28, 2012, 4:45 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Sept. 28, 2012, 4:45 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[David Faure]

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



konqueror/src/konqsessionmanager.cpp


You moved this to the init list, shouldn't it be removed from here?



konqueror/src/konqsessionmanager.cpp


This code would be better with a KDialog subclass, rather than with all the 
code in a function -- and then you need a slot in the calling class, and an 
ugly qobject_cast...

If you split this up into a proper class, then it can have its own slots 
and member variables, and this will increase modularity and reusability. (and 
readability).

Thanks.



konqueror/src/konqsessionmanager.cpp


Doesn't this lose ItemIsSelectable? I guess item->flags() | 
Qt::ItemIsUserCheckable would be better.



konqueror/src/konqsessionmanager.cpp


A QTreeWidgetItemIterator might make this code simpler (one loop instead of 
two nested loops)



konqueror/src/konqsessionmanager.cpp


This hash of hash makes the code rather complicated.
I think the goal is to associate data with each item in the list -- for 
this you can just use QTreeWidgetItem::setData(0, Qt::UserRole, 
the_internal_string)
and retrieve that again here, rather than a lookup based on the 
user-visible text (which could have duplicates)



konqueror/src/konqsessionmanager.cpp


Why did you remove the const? I don't see that you added code that changed 
that list. Probably a leftover, please revert.



konqueror/src/konqsessionmanager.cpp


Word puzzles are a big no no, in translated strings.

Use i18nc("...", "Window %1", indexOf+1);


- David Faure


On Sept. 28, 2012, 4:45 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Sept. 28, 2012, 4:45 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

> On Sept. 19, 2012, 9:28 p.m., Christoph Feck wrote:
> > Very nice. I think the text at the header could mention something along the 
> > line that you can choose which pages to restore.

Would a tooltip suffice ? See the new restore dialog v2 screenshot.


- Dawit


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


On Sept. 28, 2012, 4:45 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Sept. 28, 2012, 4:45 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> new restore dialog v2
>   http://git.reviewboard.kde.org/r/106503/s/739/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

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

(Updated Sept. 28, 2012, 4:45 p.m.)


Review request for KDE Base Apps and David Faure.


Changes
---

Use tree widget to group and display the sessions that belong to different 
windows. See v2 screenshot.


Description
---

The attached patch fixes one of those pet peeve bugs that infurate me from time 
to time by allowing me to unselect the sessions I do not want to be restored 
when Konqueror's restore session dialog pops up.


This addresses bug 260282.
http://bugs.kde.org/show_bug.cgi?id=260282


Diffs (updated)
-

  konqueror/src/konqsessionmanager.h ee629e4 
  konqueror/src/konqsessionmanager.cpp 68a003f 

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


Testing
---

* Unselected sessions should not be restored.
* If all available sessions are selected (the default), the behavior should 
remain the same as it is today.
* If all available sessions are unselected, disable the "Restore Session" 
button.


Screenshots (updated)
---

old restore dialog
  http://git.reviewboard.kde.org/r/106503/s/729/
new restore dialog
  http://git.reviewboard.kde.org/r/106503/s/731/
new restore dialog v2
  http://git.reviewboard.kde.org/r/106503/s/739/


Thanks,

Dawit Alemayehu

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Christoph Feck]

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


Very nice. I think the text at the header could mention something along the 
line that you can choose which pages to restore.

- Christoph Feck


On Sept. 19, 2012, 9:15 p.m., Dawit Alemayehu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106503/
> ---
> 
> (Updated Sept. 19, 2012, 9:15 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> ---
> 
> The attached patch fixes one of those pet peeve bugs that infurate me from 
> time to time by allowing me to unselect the sessions I do not want to be 
> restored when Konqueror's restore session dialog pops up.
> 
> 
> This addresses bug 260282.
> http://bugs.kde.org/show_bug.cgi?id=260282
> 
> 
> Diffs
> -
> 
>   konqueror/src/konqsessionmanager.h ee629e4 
>   konqueror/src/konqsessionmanager.cpp 68a003f 
> 
> Diff: http://git.reviewboard.kde.org/r/106503/diff/
> 
> 
> Testing
> ---
> 
> * Unselected sessions should not be restored.
> * If all available sessions are selected (the default), the behavior should 
> remain the same as it is today.
> * If all available sessions are unselected, disable the "Restore Session" 
> button.
> 
> 
> Screenshots
> ---
> 
> old restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/729/
> new restore dialog
>   http://git.reviewboard.kde.org/r/106503/s/731/
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

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

(Updated Sept. 19, 2012, 9:15 p.m.)


Review request for KDE Base Apps and David Faure.


Changes
---

Disable the "Restore Session" button when all available sessions are unselected.


Description
---

The attached patch fixes one of those pet peeve bugs that infurate me from time 
to time by allowing me to unselect the sessions I do not want to be restored 
when Konqueror's restore session dialog pops up.


This addresses bug 260282.
http://bugs.kde.org/show_bug.cgi?id=260282


Diffs (updated)
-

  konqueror/src/konqsessionmanager.h ee629e4 
  konqueror/src/konqsessionmanager.cpp 68a003f 

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


Testing (updated)
---

* Unselected sessions should not be restored.
* If all available sessions are selected (the default), the behavior should 
remain the same as it is today.
* If all available sessions are unselected, disable the "Restore Session" 
button.


Screenshots
---

old restore dialog
  http://git.reviewboard.kde.org/r/106503/s/729/
new restore dialog
  http://git.reviewboard.kde.org/r/106503/s/731/


Thanks,

Dawit Alemayehu

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit A]

On Wed, Sep 19, 2012 at 2:31 AM, Rolf Eike Beer 
wrote:

> > * If all available sessions are unselected,
> > presssing "Restore Session" button will behave like "Do not Restore".
>
> What about just disabling "Restore Session" then?
>

Well, I guess that can be done as well with a bit more code.

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Rolf Eike Beer]

> * If all available sessions are unselected,
> presssing "Restore Session" button will behave like "Do not Restore".

What about just disabling "Restore Session" then?

Eike

signature.asc
Description: This is a digitally signed message part.

Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Dawit Alemayehu]

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

Review request for KDE Base Apps and David Faure.


Description
---

The attached patch fixes one of those pet peeve bugs that infurate me from time 
to time by allowing me to unselect the sessions I do not want to be restored 
when Konqueror's restore session dialog pops up.


This addresses bug 260282.
http://bugs.kde.org/show_bug.cgi?id=260282


Diffs
-

  konqueror/src/konqsessionmanager.cpp 68a003f 

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


Testing
---

* Unselected sessions should not be restored.
* If all available sessions are selected (the default), the behavior should 
remain the same as it is today.
* If all available sessions are unselected, presssing "Restore Session" button 
will behave like "Do not Restore".


Screenshots
---

old restore dialog
  http://git.reviewboard.kde.org/r/106503/s/729/
new restore dialog
  http://git.reviewboard.kde.org/r/106503/s/731/


Thanks,

Dawit Alemayehu