[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