Review Request: Draft of a new AddAccountUi

[David Edmundson]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20111228/79766dd6/attachment-0001.html>