Re: Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.
--- 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
Re: Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.
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 !DOCTYPE html PUBLIC -//W3C//DTD XHTML 1.1//EN http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd; html xmlns=http://www.w3.org/1999/xhtml; xml:lang=en head titleDisabled button test/title meta http-equiv=content-type content=text/html; charset=utf-8 / /head body table trth colspan=2With table around/th/tr tr tdlabel for=n_nameName:/label/td tdinput id=n_name type=text name=n_name size=60 value=Entry that needs some space readonly=true //td /tr /table input id=n_name2 type=text name=n_name2 size=60 value=Another entry that needs some space readonly=true / /body /html
Re: Review Request: Fix logic with clear-button animation in klineedit (notably at first try) and address bug 268898.
On July 26, 2011, 10:46 p.m., Aleix Pol Gonzalez wrote: kdeui/widgets/klineedit.cpp, line 358 http://git.reviewboard.kde.org/r/102095/diff/1/?file=30032#file30032line358 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.
On July 26, 2011, 10:46 p.m., Aleix Pol Gonzalez wrote: kdeui/widgets/klineedit.cpp, line 358 http://git.reviewboard.kde.org/r/102095/diff/1/?file=30032#file30032line358 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
