> On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote: > > src/KCMTelepathyAccounts/feedback-widget.cpp, line 74 > > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8312#file8312line74> > > > > Not sure this is right. event->rect() is the area to be repainted not > > always the whole area of the widget, if you redrew half of it, all your > > rounded corners would be wrong. > > > > Just > > r = rect(); would get the widget area. > > > > then you can do > > > > event->rect() is normally used in: > > p.setClippingRect(event->rect()); > >
I agree on this one. The code was copied from the bluetooth dialog, didn't really check it. > On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote: > > src/KCMTelepathyAccounts/feedback-widget.cpp, line 88 > > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8312#file8312line88> > > > > coding convention is normally > > > > case Foo: > > myStuff(); > > break; Fixed > On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote: > > src/KCMTelepathyAccounts/feedback-widget.cpp, line 92 > > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8312#file8312line92> > > > > Does KColorScheme/QPallete have a better way of selected a warning > > color? Seems like there is a color for that: "NeutralBackground : Seventh color; for example, warnings, secure/encrypted content." The yellow for warning seems ok. I can't really find one for InfoMessage though > On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote: > > src/KCMTelepathyAccounts/feedback-widget.cpp, line 97 > > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8312#file8312line97> > > > > I don't understand what this is achieving. > > > > > > Possibly a side effect of the rect comment above. I think you're right. This was copied as well. When using rect(), this doesn't seem to have any effect. > On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote: > > src/KCMTelepathyAccounts/validated-line-edit.cpp, line 163 > > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8317#file8317line163> > > > > If you stored KIcons instead of QPixmaps I think you wouldn't need to > > do resizing, you could just export to the correctly sized pixmap to start > > with. Why would that be better? I assume that textboxes are not equally large in every style/with every font. You'd still need to convert them to a pixmap > On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote: > > src/KCMTelepathyAccounts/validated-line-edit.cpp, line 176 > > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8317#file8317line176> > > > > there's little point writing > > > > this->doSomething(); > > when you can write > > > > doSomething(); Agreed. I usually do this because i think it reads better. Anyway, will remove it. > On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote: > > src/add-account-assistant.cpp, line 204 > > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8319#file8319line204> > > > > Not sure I like this message. It doesn't say how to proceed. > > > > A user should never see the name telepathy. Not that they should ever > > see this message. > > > > Maybe something more generic on the lines of "internal error, check > > your system setup". With this message people can at least try to find help. "Internal error" says nothing at all, not even what went wrong. > On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote: > > src/edit-account-dialog.cpp, line 91 > > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8320#file8320line91> > > > > Why does this need moving? > > > > I can't really remember but I'm pretty sure it had a reason. I think it had something to with making sure that data is submitted to the model before validating. - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100455/#review1285 ----------------------------------------------------------- On Feb. 3, 2011, 7:41 p.m., Thomas Richard wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/100455/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2011, 7:41 p.m.) > > > Review request for Telepathy. > > > Summary > ------- > > This patch will give some visible feedback in a non obtrusive way. There is > also the possibility to provide some custom validation on parameters using > the ParameterEditModel. > > The screenshots speak a 1000 words ;) > > > Diffs > ----- > > src/KCMTelepathyAccounts/CMakeLists.txt > 51389226299db38012f7f326b3e57709f4d3e244 > src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp > 9f5b0d4f9b27c0502783895ee0f52e320829ffc3 > src/KCMTelepathyAccounts/account-edit-widget.h > 11d80778e4ed0fc7f80943d2248d3f08b27feb20 > src/KCMTelepathyAccounts/account-edit-widget.cpp > 5c09e5dade33ae383d63827add757c8688c24fb7 > src/KCMTelepathyAccounts/feedback-widget.h PRE-CREATION > src/KCMTelepathyAccounts/feedback-widget.cpp PRE-CREATION > src/KCMTelepathyAccounts/include/ValidatedLineEdit PRE-CREATION > src/KCMTelepathyAccounts/parameter-edit-model.h > d904facc2c958440d193fe86182bd50c0ee6721f > src/KCMTelepathyAccounts/parameter-edit-model.cpp > 441b6e58daf13b3fcf65ab344544ae36e50b940b > src/KCMTelepathyAccounts/validated-line-edit.h PRE-CREATION > src/KCMTelepathyAccounts/validated-line-edit.cpp PRE-CREATION > src/add-account-assistant.h 973bad46e11697135c331c632e9cb63dcb790232 > src/add-account-assistant.cpp eb9a644d38b0020ce3158c99f3714dddd1b8047a > src/edit-account-dialog.cpp bcfaca26ec9fbf8385b4d43d2c673319c8bdc9a9 > > Diff: http://git.reviewboard.kde.org/r/100455/diff > > > Testing > ------- > > When all parameters are valid, an account still gets added > > > Screenshots > ----------- > > Valid email address > http://git.reviewboard.kde.org/r/100455/s/56/ > Invalid email address > http://git.reviewboard.kde.org/r/100455/s/57/ > Invalid email address and clicking apply > http://git.reviewboard.kde.org/r/100455/s/58/ > When an account did not get accepted by telepathy itself > http://git.reviewboard.kde.org/r/100455/s/59/ > > > Thanks, > > Thomas > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
