[kopete-devel] Review Request: Replaced KPassivePopup notifications with KNotification.

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

Review request for Kopete.


Summary
-------

I've replaced all the KPassivePopup with KNotifications.


Diffs
-----

  /trunk/KDE/kdenetwork/kopete/protocols/groupwise/gwaccount.cpp 1025366
  /trunk/KDE/kdenetwork/kopete/protocols/jabber/jabberaccount.cpp 1025366
  /trunk/KDE/kdenetwork/kopete/protocols/oscar/oscaraccount.cpp 1025366

Diff: http://reviewboard.kde.org/r/1661/diff


Testing
-------


Thanks,

Davide

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel

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



/trunk/KDE/kdenetwork/kopete/protocols/groupwise/gwaccount.cpp
<http://reviewboard.kde.org/r/1661/#comment1708>

    I've never used KNotification, so please correct me if I'm wrong.
   
    Do you really need to add "Kopete: " to the title of the notification? Does it mean if you don't the user cannot know the notification was brought up by Kopete?
   
    This applies to the other places you add "Kopete: " to the titles too.



/trunk/KDE/kdenetwork/kopete/protocols/oscar/oscaraccount.cpp
<http://reviewboard.kde.org/r/1661/#comment1709>

    This applies to the other changes to this specific file.
   
    I assume you've missed that the function here is i18nc, not i18n. In this case, you're adding "Kopete: " to the context message the translators are going to read when translating the messages, which doesn't make sense.
   
    You probably wanted to change the title as you did with the other files; in this case, change the second function argument. My question about prepending "Kopete: " to the titles still applies, though.


- Raphael


On 2009-09-20 13:53:02, Davide Bettio wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1661/
> -----------------------------------------------------------
>
> (Updated 2009-09-20 13:53:02)
>
>
> Review request for Kopete.
>
>
> Summary
> -------
>
> I've replaced all the KPassivePopup with KNotifications.
>
>
> Diffs
> -----
>
>   /trunk/KDE/kdenetwork/kopete/protocols/groupwise/gwaccount.cpp 1025366
>   /trunk/KDE/kdenetwork/kopete/protocols/jabber/jabberaccount.cpp 1025366
>   /trunk/KDE/kdenetwork/kopete/protocols/oscar/oscaraccount.cpp 1025366
>
> Diff: http://reviewboard.kde.org/r/1661/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Davide
>
>

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel

> On 2009-09-21 02:24:51, Raphael Kubo da Costa wrote:
> > /trunk/KDE/kdenetwork/kopete/protocols/groupwise/gwaccount.cpp, line 586
> > <http://reviewboard.kde.org/r/1661/diff/1/?file=11705#file11705line586>
> >
> >     I've never used KNotification, so please correct me if I'm wrong.
> >    
> >     Do you really need to add "Kopete: " to the title of the notification? Does it mean if you don't the user cannot know the notification was brought up by Kopete?
> >    
> >     This applies to the other places you add "Kopete: " to the titles too.

yes, I think so


> On 2009-09-21 02:24:51, Raphael Kubo da Costa wrote:
> > /trunk/KDE/kdenetwork/kopete/protocols/oscar/oscaraccount.cpp, line 1006
> > <http://reviewboard.kde.org/r/1661/diff/1/?file=11707#file11707line1006>
> >
> >     This applies to the other changes to this specific file.
> >    
> >     I assume you've missed that the function here is i18nc, not i18n. In this case, you're adding "Kopete: " to the context message the translators are going to read when translating the messages, which doesn't make sense.
> >    
> >     You probably wanted to change the title as you did with the other files; in this case, change the second function argument. My question about prepending "Kopete: " to the titles still applies, though.

right. I'm going to change it.


- Davide


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


On 2009-09-20 13:53:02, Davide Bettio wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1661/
> -----------------------------------------------------------
>
> (Updated 2009-09-20 13:53:02)
>
>
> Review request for Kopete.
>
>
> Summary
> -------
>
> I've replaced all the KPassivePopup with KNotifications.
>
>
> Diffs
> -----
>
>   /trunk/KDE/kdenetwork/kopete/protocols/groupwise/gwaccount.cpp 1025366
>   /trunk/KDE/kdenetwork/kopete/protocols/jabber/jabberaccount.cpp 1025366
>   /trunk/KDE/kdenetwork/kopete/protocols/oscar/oscaraccount.cpp 1025366
>
> Diff: http://reviewboard.kde.org/r/1661/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Davide
>
>

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel

> On 2009-09-21 02:24:51, Raphael Kubo da Costa wrote:
> > /trunk/KDE/kdenetwork/kopete/protocols/groupwise/gwaccount.cpp, line 586
> > <http://reviewboard.kde.org/r/1661/diff/1/?file=11705#file11705line586>
> >
> >     I've never used KNotification, so please correct me if I'm wrong.
> >    
> >     Do you really need to add "Kopete: " to the title of the notification? Does it mean if you don't the user cannot know the notification was brought up by Kopete?
> >    
> >     This applies to the other places you add "Kopete: " to the titles too.
>
>  wrote:
>     yes, I think so

Then isn't it better to set the pixmap parameter to Kopete's icon instead? Requiring "Kopete: " in all titles may be too error-prone, and developers might forget it when adding new messages.

Other possibilities are creating a function that always does this, or documenting this need somewhere.

Anyway, it would be nice if other people commented on this as well.


- Raphael


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


On 2009-09-20 13:53:02, Davide Bettio wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1661/
> -----------------------------------------------------------
>
> (Updated 2009-09-20 13:53:02)
>
>
> Review request for Kopete.
>
>
> Summary
> -------
>
> I've replaced all the KPassivePopup with KNotifications.
>
>
> Diffs
> -----
>
>   /trunk/KDE/kdenetwork/kopete/protocols/groupwise/gwaccount.cpp 1025366
>   /trunk/KDE/kdenetwork/kopete/protocols/jabber/jabberaccount.cpp 1025366
>   /trunk/KDE/kdenetwork/kopete/protocols/oscar/oscaraccount.cpp 1025366
>
> Diff: http://reviewboard.kde.org/r/1661/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Davide
>
>

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel