[prev in list] [next in list] [prev in thread] [next in thread]
List: kopete-devel
Subject: Re: Review Request: Add support for mpris2 in nowlistening-plugin
From: "Lamarque V. Souza" <lamarque () kde ! org>
Date: 2012-05-31 0:29:19
Message-ID: 201205302129.20018.lamarque () kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Em Wednesday 30 May 2012, cyberbeat@gmx.de escreveu:
> > On May 31, 2012, 12:01 a.m., Lamarque Souza wrote:
> Thanks for the quick review. I'll fix the other issues soon.
>
> Do you also experience, that you cannot choose in nowlistening preferences
> the media-player, when you do not modify the checkbox "use special
> mediaplayer.."?
You mean if "use special mediaplayer" is unchecked then you cannot
select the media-player? Yes, that happens here but it seems logical. If you
do not want a special media player why let the list enabled?
> I fixed that by using a klistwidget instead of k3listbox. KCMModule seems
> to not recognizes changes on k3listbox-selections.
>
> > On May 31, 2012, 12:01 a.m., Lamarque Souza wrote:
> > > /trunk/KDE/kdenetwork/kopete/plugins/nowlistening/nlmpris2.cpp, line 61
> > > <http://svn.reviewboard.kde.org/r/6960/diff/1/?file=48034#file48034line
> > > 61>
> > >
> > > This implementation is basically a copy of the mpris plugin
> > > replacing the dbus service and interface names. In the original
> > > mpris implementation this line search for the string "org.mpris."
> > > and not "org.mpris.MediaPlayer". Now I am in doubt if this line
> > > should also be "org.mpris." or the other line should be
> > > "org.mpris.MediaPlayer". I am not an expert in mpris so I do not
> > > know each one should be used here.
>
> I think the original Implementation is not good, because it will also find
> mpris2 Interface, but cannot use it.
Ok, thanks for the clarification. Well, looking again at the patch there
are better ways to implement the m_client management. For example, you can use
QDBusServiceWatcher to check when the service org.mpris.MediaPlayer2
disappears, that way you will not need to delete and allocate memory for
m_client everytime you update the song info, which happens every 5s if I
recall correctly.
I am also not fan of QDBusReply (and waitForFinished for the matter).
They block the event loop while they are processed, which means a problem in
the player can freeze the nowlistening plugin. I do not know if a Kopete
plugin can freeze the entire Kopete, but anyway, it is better not take the
chance.
--
Lamarque V. Souza
KDE's Network Management maintainer
http://planetkde.org/pt-br
[Attachment #5 (text/html)]
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" "http://www.w3.org/TR/REC-html40/strict.dtd">
<html><head><meta name="qrichtext" content="1" /><style type="text/css">
p, li { white-space: pre-wrap; }
</style></head><body style=" font-family:'Tahoma'; font-size:12pt; font-weight:400; \
font-style:normal;"> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">Em Wednesday 30 May \
2012, cyberbeat@gmx.de escreveu:</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> \
> On May 31, 2012, 12:01 a.m., Lamarque Souza wrote:</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> Thanks for the quick review. I'll fix the other issues soon.</p> <p \
style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> </p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> Do you also experience, that you cannot choose in nowlistening \
preferences</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> the \
media-player, when you do not modify the checkbox "use special</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> mediaplayer.."?</p> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"> You mean if "use special mediaplayer" is unchecked then you \
cannot select the media-player? Yes, that happens here but it seems logical. If you do not want \
a special media player why let the list enabled?</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"> </p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> I fixed that by \
using a klistwidget instead of k3listbox. KCMModule seems</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> to not recognizes changes on k3listbox-selections.</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> </p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> \
> On May 31, 2012, 12:01 a.m., Lamarque Souza wrote:</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> > > \
/trunk/KDE/kdenetwork/kopete/plugins/nowlistening/nlmpris2.cpp, line 61</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > > \
<http://svn.reviewboard.kde.org/r/6960/diff/1/?file=48034#file48034line</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > > 61></p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> > > </p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> \
> > This implementation is basically a copy of the mpris plugin</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > > replacing the dbus service and interface \
names. In the original</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > > \
mpris implementation this line search for the string "org.mpris."</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > > and not \
"org.mpris.MediaPlayer". Now I am in doubt if this line</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > > should also be "org.mpris." or \
the other line should be</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > > \
"org.mpris.MediaPlayer". I am not an expert in mpris so I do not</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > > know each one should be used here.</p> \
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> </p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> I think the original Implementation is not good, because it will also \
find</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> mpris2 Interface, but cannot use \
it.</p> <p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"> Ok, thanks for the clarification. Well, looking again at the patch there \
are better ways to implement the m_client management. For example, you can use \
QDBusServiceWatcher to check when the service org.mpris.MediaPlayer2 disappears, that way you \
will not need to delete and allocate memory for m_client everytime you update the song info, \
which happens every 5s if I recall correctly.</p> <p style="-qt-paragraph-type:empty; \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; "> </p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> I am also not fan of \
QDBusReply (and waitForFinished for the matter). They block the event loop while they are \
processed, which means a problem in the player can freeze the nowlistening plugin. I do not \
know if a Kopete plugin can freeze the entire Kopete, but anyway, it is better not take the \
chance.</p> <p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">-- </p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">Lamarque V. Souza</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">KDE's Network Management maintainer</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">http://planetkde.org/pt-br</p></body></html>
_______________________________________________
kopete-devel mailing list
kopete-devel@kde.org
https://mail.kde.org/mailman/listinfo/kopete-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic