Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.

[Hugo Pereira Da Costa]

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

Review request for kdelibs.


Summary
---

Details:
- fixes the somewhat incorrect logic in KLineEditButton::animateVisible
- simplifies KLineEdit::updateClearButtonIcon consequently.


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


Diffs
-

  kdeui/widgets/klineedit.cpp 8f1c8a4 
  kdeui/widgets/klineedit_p.h 95016bd 

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


Testing
---

tested with klineedittest found in kdelibs/kdeui/tests, this with and without 
the patch attached to comment #1 of bug 268898, used to actually trigger the 
mentionned bug. Also tested with other klineEdit implementation such as 
Dolphin's location bar.


Thanks,

Hugo

Re: Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.

[Aleix Pol Gonzalez]

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



kdeui/widgets/klineedit.cpp


Wouldn't it be better to put it this way? Just saying...

d->clearButton->animateVisible(d->wideEnoughForClear && text.length() > 0);


- Aleix


On July 26, 2011, 9:54 p.m., Hugo Pereira Da Costa wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102095/
> ---
> 
> (Updated July 26, 2011, 9:54 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> Details:
> - fixes the somewhat incorrect logic in KLineEditButton::animateVisible
> - simplifies KLineEdit::updateClearButtonIcon consequently.
> 
> 
> This addresses bug 268898.
> http://bugs.kde.org/show_bug.cgi?id=268898
> 
> 
> Diffs
> -
> 
>   kdeui/widgets/klineedit.cpp 8f1c8a4 
>   kdeui/widgets/klineedit_p.h 95016bd 
> 
> Diff: http://git.reviewboard.kde.org/r/102095/diff
> 
> 
> Testing
> ---
> 
> tested with klineedittest found in kdelibs/kdeui/tests, this with and without 
> the patch attached to comment #1 of bug 268898, used to actually trigger the 
> mentionned bug. Also tested with other klineEdit implementation such as 
> Dolphin's location bar.
> 
> 
> Thanks,
> 
> Hugo
> 
>

Re: Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.

[Rolf Eike Beer]

> Details:
> - fixes the somewhat incorrect logic in KLineEditButton::animateVisible
> - simplifies KLineEdit::updateClearButtonIcon consequently.

Please test this also when using Konqueror and edit fields (e.g. login
boxes). There have been some bad regressions about KLineEdit popping up in
Konqueror, e.g. bug:246513. There is also one more regression about the
return handling that I can't find at the moment (Andrea?).

And while I'm at it here is another one: the following HTML fragment will
have a damaged drawing of the line edits which will go away when you hover
the broken place with the mouse. For me it looks like the clear button was
originally drawn at that place and then it was removed because the input
is disabled. I only managed to create a small self-contained testcase
right now so there is no bug report yet. Will add that tonight.

Eike

http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd";>
http://www.w3.org/1999/xhtml"; xml:lang="en">

Disabled button test




With table around

Name:

Re: Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.

[Nicolas Alvarez]

> On July 26, 2011, 10:46 p.m., Aleix Pol Gonzalez wrote:
> > kdeui/widgets/klineedit.cpp, line 358
> > 
> >
> > Wouldn't it be better to put it this way? Just saying...
> > 
> > d->clearButton->animateVisible(d->wideEnoughForClear && text.length() > 
> > 0);

I think the original is clearer, to be honest.


- Nicolas


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


On July 26, 2011, 9:54 p.m., Hugo Pereira Da Costa wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102095/
> ---
> 
> (Updated July 26, 2011, 9:54 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> Details:
> - fixes the somewhat incorrect logic in KLineEditButton::animateVisible
> - simplifies KLineEdit::updateClearButtonIcon consequently.
> 
> 
> This addresses bug 268898.
> http://bugs.kde.org/show_bug.cgi?id=268898
> 
> 
> Diffs
> -
> 
>   kdeui/widgets/klineedit.cpp 8f1c8a4 
>   kdeui/widgets/klineedit_p.h 95016bd 
> 
> Diff: http://git.reviewboard.kde.org/r/102095/diff
> 
> 
> Testing
> ---
> 
> tested with klineedittest found in kdelibs/kdeui/tests, this with and without 
> the patch attached to comment #1 of bug 268898, used to actually trigger the 
> mentionned bug. Also tested with other klineEdit implementation such as 
> Dolphin's location bar.
> 
> 
> Thanks,
> 
> Hugo
> 
>

Re: Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.

[Hugo Pereira Da Costa]

> On July 26, 2011, 10:46 p.m., Aleix Pol Gonzalez wrote:
> > kdeui/widgets/klineedit.cpp, line 358
> > 
> >
> > Wouldn't it be better to put it this way? Just saying...
> > 
> > d->clearButton->animateVisible(d->wideEnoughForClear && text.length() > 
> > 0);
> 
> Nicolas Alvarez wrote:
> I think the original is clearer, to be honest.

I agree with Nicolas.
I have nothing against putting boolean test inside the method call, *in 
principle*, but I believe it's convenient only if the boolean condition is 
"short enough" to write. 


- Hugo


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


On July 26, 2011, 9:54 p.m., Hugo Pereira Da Costa wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102095/
> ---
> 
> (Updated July 26, 2011, 9:54 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> Details:
> - fixes the somewhat incorrect logic in KLineEditButton::animateVisible
> - simplifies KLineEdit::updateClearButtonIcon consequently.
> 
> 
> This addresses bug 268898.
> http://bugs.kde.org/show_bug.cgi?id=268898
> 
> 
> Diffs
> -
> 
>   kdeui/widgets/klineedit.cpp 8f1c8a4 
>   kdeui/widgets/klineedit_p.h 95016bd 
> 
> Diff: http://git.reviewboard.kde.org/r/102095/diff
> 
> 
> Testing
> ---
> 
> tested with klineedittest found in kdelibs/kdeui/tests, this with and without 
> the patch attached to comment #1 of bug 268898, used to actually trigger the 
> mentionned bug. Also tested with other klineEdit implementation such as 
> Dolphin's location bar.
> 
> 
> Thanks,
> 
> Hugo
> 
>

Re: Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.

[Hugo Pereira Da Costa]

> On July 26, 2011, 10:46 p.m., Aleix Pol Gonzalez wrote:
> > kdeui/widgets/klineedit.cpp, line 358
> > 
> >
> > Wouldn't it be better to put it this way? Just saying...
> > 
> > d->clearButton->animateVisible(d->wideEnoughForClear && text.length() > 
> > 0);
> 
> Nicolas Alvarez wrote:
> I think the original is clearer, to be honest.
> 
> Hugo Pereira Da Costa wrote:
> I agree with Nicolas.
> I have nothing against putting boolean test inside the method call, *in 
> principle*, but I believe it's convenient only if the boolean condition is 
> "short enough" to write.

Other than that, anyone willing to go for a "ship it" ? 
It was reported on bug 268898 that the patch actually works, and that no 
regression has been found so far (which is also my own experience)


- Hugo


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


On July 26, 2011, 9:54 p.m., Hugo Pereira Da Costa wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102095/
> ---
> 
> (Updated July 26, 2011, 9:54 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> Details:
> - fixes the somewhat incorrect logic in KLineEditButton::animateVisible
> - simplifies KLineEdit::updateClearButtonIcon consequently.
> 
> 
> This addresses bug 268898.
> http://bugs.kde.org/show_bug.cgi?id=268898
> 
> 
> Diffs
> -
> 
>   kdeui/widgets/klineedit.cpp 8f1c8a4 
>   kdeui/widgets/klineedit_p.h 95016bd 
> 
> Diff: http://git.reviewboard.kde.org/r/102095/diff
> 
> 
> Testing
> ---
> 
> tested with klineedittest found in kdelibs/kdeui/tests, this with and without 
> the patch attached to comment #1 of bug 268898, used to actually trigger the 
> mentionned bug. Also tested with other klineEdit implementation such as 
> Dolphin's location bar.
> 
> 
> Thanks,
> 
> Hugo
> 
>

Re: Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.

[Dawit Alemayehu]

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

Ship it!


So long as the patch does not cause regressions like the one mentioned by Rolf 
(bug 246513), then feel free to ship it.

- Dawit


On July 26, 2011, 9:54 p.m., Hugo Pereira Da Costa wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102095/
> ---
> 
> (Updated July 26, 2011, 9:54 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> Details:
> - fixes the somewhat incorrect logic in KLineEditButton::animateVisible
> - simplifies KLineEdit::updateClearButtonIcon consequently.
> 
> 
> This addresses bug 268898.
> http://bugs.kde.org/show_bug.cgi?id=268898
> 
> 
> Diffs
> -
> 
>   kdeui/widgets/klineedit.cpp 8f1c8a4 
>   kdeui/widgets/klineedit_p.h 95016bd 
> 
> Diff: http://git.reviewboard.kde.org/r/102095/diff
> 
> 
> Testing
> ---
> 
> tested with klineedittest found in kdelibs/kdeui/tests, this with and without 
> the patch attached to comment #1 of bug 268898, used to actually trigger the 
> mentionned bug. Also tested with other klineEdit implementation such as 
> Dolphin's location bar.
> 
> 
> Thanks,
> 
> Hugo
> 
>

Re: Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.

[Commit Hook]

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


This review has been submitted with commit 
a456e8600b63300d520b074a6d71d74219df3058 by Hugo Pereira Da Costa to branch 
master.

- Commit


On July 26, 2011, 9:54 p.m., Hugo Pereira Da Costa wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102095/
> ---
> 
> (Updated July 26, 2011, 9:54 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> Details:
> - fixes the somewhat incorrect logic in KLineEditButton::animateVisible
> - simplifies KLineEdit::updateClearButtonIcon consequently.
> 
> 
> This addresses bug 268898.
> http://bugs.kde.org/show_bug.cgi?id=268898
> 
> 
> Diffs
> -
> 
>   kdeui/widgets/klineedit.cpp 8f1c8a4 
>   kdeui/widgets/klineedit_p.h 95016bd 
> 
> Diff: http://git.reviewboard.kde.org/r/102095/diff
> 
> 
> Testing
> ---
> 
> tested with klineedittest found in kdelibs/kdeui/tests, this with and without 
> the patch attached to comment #1 of bug 268898, used to actually trigger the 
> mentionned bug. Also tested with other klineEdit implementation such as 
> Dolphin's location bar.
> 
> 
> Thanks,
> 
> Hugo
> 
>