----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103562/#review9325 -----------------------------------------------------------
Great start, needs a second review though. src/KCMTelepathyAccounts/profile-item.cpp <http://git.reviewboard.kde.org/r/103562/#comment7695> No point having the same method twice. Delete the original constructor. src/KCMTelepathyAccounts/simple-profile-select-widget.h <http://git.reviewboard.kde.org/r/103562/#comment7694> This isn't you. src/KCMTelepathyAccounts/simple-profile-select-widget.h <http://git.reviewboard.kde.org/r/103562/#comment7696> For future reference look up either QSignalMapper or QButtonGroup then you can just have "onProfileClicked(int id);", which you can load from a map, QSignalMapper can even provide the string. Though there's no need to change this now. src/KCMTelepathyAccounts/simple-profile-select-widget.h <http://git.reviewboard.kde.org/r/103562/#comment7697> we don't tend to have words like got/was in the signals/slots. src/KCMTelepathyAccounts/simple-profile-select-widget.cpp <http://git.reviewboard.kde.org/r/103562/#comment7698> This still isn't you. src/KCMTelepathyAccounts/simple-profile-select-widget.cpp <http://git.reviewboard.kde.org/r/103562/#comment7701> Do you use the model? If not, don't create them! src/KCMTelepathyAccounts/simple-profile-select-widget.cpp <http://git.reviewboard.kde.org/r/103562/#comment7704> We need to check these exist. Even if we just run setEnabled(profileManager->profileForService("im-jabber").isValid()); Otherwise things will go seriously wrong. src/KCMTelepathyAccounts/simple-profile-select-widget.cpp <http://git.reviewboard.kde.org/r/103562/#comment7699> If your slot only emits a signal you can connect it directly connect(buttonOThers, SIGNAL(clicked()), SIGNAL(othersGotChosen()); src/KCMTelepathyAccounts/simple-profile-select-widget.cpp <http://git.reviewboard.kde.org/r/103562/#comment7702> "foo" ?_? src/add-account-assistant.cpp <http://git.reviewboard.kde.org/r/103562/#comment7703> Oh this is bad, because you're new selectedProfile() creates an object, running this will creates two ! src/add-account-assistant.cpp <http://git.reviewboard.kde.org/r/103562/#comment7700> coding style if (foo) { //remember braces } - David Edmundson On Dec. 28, 2011, 2:28 a.m., Florian Reinhard wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103562/ > ----------------------------------------------------------- > > (Updated Dec. 28, 2011, 2:28 a.m.) > > > Review request for Telepathy. > > > Description > ------- > > This patch is not intended for master. Consider it to be a more detailed > mockup, I know the code is crappy ;) > > Draw backs: > * crashes from time to time because of ugly hacks in ProfileItem / > SimpleProfileSelectWidget::selectedProfile() > * profile names are hardcoded, I guess this shouldn't stay this way? > * profiles from the first page are not excluded from the second page > * the naming of page one, two, three is hard to understand when reading the > code > * if you click on "Other" you get was formerly was the entry page, one could > use the new shiny buttons there too. > > > This addresses bug 279046. > http://bugs.kde.org/show_bug.cgi?id=279046 > > > Diffs > ----- > > src/KCMTelepathyAccounts/CMakeLists.txt > 0991b4dd3a65d34746e3c92d09c43c4cbced8e92 > src/KCMTelepathyAccounts/profile-item.h > dc492a5e37192d91833348e1e1dfe9c96fe41f51 > src/KCMTelepathyAccounts/profile-item.cpp > de42b521cbfb047dabf1c5c85decc47dceaac54e > src/KCMTelepathyAccounts/profile-select-widget.h > 52c6f898728dab5ddd8c73548caeea99ac271efe > src/KCMTelepathyAccounts/profile-select-widget.cpp > dc8b9cc1d0bad9c856834c4e0dbae7b9102dee89 > src/KCMTelepathyAccounts/simple-profile-select-widget.h PRE-CREATION > src/KCMTelepathyAccounts/simple-profile-select-widget.cpp PRE-CREATION > src/KCMTelepathyAccounts/simple-profile-select-widget.ui PRE-CREATION > src/add-account-assistant.h 9be243ffbe2d246042ed3684f696f0fccbc1252e > src/add-account-assistant.cpp dee6e88c51c72f36a1e3099a272db71443a7194f > > Diff: http://git.reviewboard.kde.org/r/103562/diff/diff > > > Testing > ------- > > > Screenshots > ----------- > > > http://git.reviewboard.kde.org/r/103562/s/384/ > > > Thanks, > > Florian Reinhard > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
