Re: [kaffeine-devel] dbus support for kaffeine
Brought to you by:
hftom,
lasselindqvist
Menu
▾
▴
Re: [kaffeine-devel] dbus support for kaffeine
|
From: Christoph P. <chr...@gm...> - 2009-11-30 17:15:15
|
Hi,
2009/11/24 Pascal <pa...@bo...>:
> Hi Christoph,
>
> with my old patch the channel ui didn't get updated when the channel was set
> using the SetNumber(int) function, i now fixed it. I also cleaned up a
> little bit my code in dvbtab.cpp and fixed a possible segmentation fault
> when the dbus function SetDvbChannel(Qstring) was called with an invalid
> name. The patch is against rev. 1053875.
Finally I managed to review, overwork and commit your patch ...
> cheers,
> pascal
> +class MprisPlayerObject : public QObject
> +{
> + Q_OBJECT
> + Q_CLASSINFO("D-Bus Interface", "org.freedesktop.MediaPlayer")
> +public:
> + explicit MprisPlayerObject(MediaWidget *mediaWidget_);
> + ~MprisPlayerObject();
> +
> +public slots:
> + void Next();
> + void Prev();
> + void Play();
> + void Pause();
> + void Stop();
> + int VolumeGet();
> + void VolumeSet(int volume);
> + int PositionGet();
> + void PositionSet(int position);
> + void Repeat();
You forgot the bool parameter.
> + MprisStatusStruct GetStatus();
> + QVariantMap GetMetadata();
MPRIS metadata and Phonon metadata specifications aren't 100%
compatible ("location" means url in mpris terms, while it means
"location where track was recorded" in Phonon terms) ... I've left out
the metadata part for the moment.
> + int GetCaps();
All functions reordered into the order given by the spec.
> + // this functions are not part of the MPRIS specs
> + void PlayPause();
Not needed, Pause() has the same behaviour than mediaWidget's playPause action.
> + void VolumeIncrease();
> + void VolumeDecrease();
Renamed to "IncreaseVolume" and "DecreaseVolume".
> + void Play(const QString &url);
Not needed, TrackList has an AddTrack() function.
> + void PlayAudioCd();
> + void PlayVideoCd();
> + void PlayDvd();
> + void ChangeAudioChannel(int index);
> + void ChangeSubtitle(int index);
> + void LongSkipBackward();
> + void SkipBackward();
> + void SkipForward();
> + void LongSkipForward();
> + void TimeButtonClicked();
Left out for the moment (this has much to do with my workflow, not
with the actual content of the patch ... I'll think about those
functions once I have some spare time again).
> + void ToggleMuted();
> + void UpdateTimeButton();
Not needed, internal function.
> +signals:
> + void CapsChange( int );
> + void StatusChange( MprisStatusStruct );
> + void TrackChange( QVariantMap );
Reordered into the order given by the spec.
> +private:
> + MediaWidget *mediaWidget;
> +class MprisTrackListObject : public QObject
> +{
> + Q_OBJECT
> + Q_CLASSINFO("D-Bus Interface", "org.freedesktop.MediaPlayer")
> +public:
> + explicit MprisTrackListObject(MediaWidget *mediaWidget_);
> + ~MprisTrackListObject();
> +
> +public slots:
> + QVariantMap GetMetadata();
> + int GetCurrentTrack();
> + int GetLength();
> + int AddTrack(QString track, bool play);
Use "const QString &", not "QString".
> + void DelTrack(int position);
> + void SetLoop(bool loop);
> + void SetRandom(bool random);
> +
> +signals:
> + void TrackListChange(int);
> +
> +private:
> + MediaWidget *mediaWidget;
All those functions have a relation to PlaylistTab, not to MediaWidget.
> +class MprisDvbObject : public QObject
Renamed to "DBusTelevisionObject" (nothing to do with mpris).
> +{
> + Q_OBJECT
> + Q_CLASSINFO("D-Bus Interface", "org.freedesktop.MediaPlayer")
> +public:
> + explicit MprisDvbObject(MediaWidget *mediaWidget_);
> + ~MprisDvbObject();
> +
> +public slots:
> + void SetDvbChannel(const QString &name);
> + void SetDvbChannelNumber(int number);
Merged into a single function - playChannel(const QString &nameOrNumber);
> + void SetNumber(int number);
Not needed (what's the sense of this function?)
Added a DigitPressed(int) function, for entering channel numbers via
remote control.
> + void SetDvbLastChannel();
Renamed to PlayLastChannel().
> + void StopDvb();
Not needed, internal function.
> + void ToggleInstantRecord();
> + void ToggleOsd();
> +
> +private:
> + MediaWidget *mediaWidget;
All those functions have a relation to DvbTab, not to MediaWidget.
> + void AspectRatioAuto();
> + void AspectRatio4_3();
> + void AspectRatio16_9();
> + void AspectRatioWidget();
Left out for the moment (this has much to do with my workflow, not
with the actual content of the patch ... I'll think about those
functions once I have some spare time again).
> + void ToggleFullScreen();
Moved to the player object.
> +struct MprisStatusStruct
> +{
> + quint16 play;
> + quint16 random;
> + quint16 repeatTrack;
> + quint16 repeatAll;
> +};
The spec says 32 bit integers (= int).
> +Q_DECLARE_METATYPE(MprisStatusStruct)
> +
> +/**
> +* The bit values for the capabilities flags
> +*/
> +enum Cap {
> + NO_CAPS = 0,
> + CAN_GO_NEXT = 1 << 0,
> + CAN_GO_PREV = 1 << 1,
> + CAN_PAUSE = 1 << 2,
> + CAN_PLAY = 1 << 3,
> + CAN_SEEK = 1 << 4,
> + CAN_PROVIDE_METADATA = 1 << 5,
> + CAN_HAS_TRACKLIST = 1 << 6,
> + ALL_KNOWN_CAPS = (1 << 7) - 1
> +};
> +Q_DECLARE_FLAGS(Caps, Cap)
Directly merged into GetCaps (because it is the only function that
will use caps).
> +signals:
> + void updateDvbChannelUi(int number);
Solved that problem in a different way (ChannelModel returns the row,
not the channel itself --> the proxy model can map that information to
the correct view model index).
> +void DvbTab::toggleInstantRecord()
> +{
> + if (instantRecordAction->isChecked()) {
> + instantRecordAction->setChecked(false);
> + instantRecord(false);
> + }
> + else {
> + instantRecordAction->setChecked(true);
> + instantRecord(true);
> + }
Simply use instantRecordAction->trigger();
> +}
> +bool MediaWidget::isPaused()
> +{
> + if (playing && mediaObject->state() == Phonon::PausedState)
> + {
> + return true;
> + }
> + return false;
Query the playPauseAction.
> +}
> +int MediaWidget::getVolume()
> +{
> + int volume = audioOutput->volume() * 100;
> +
> + if ( volume >= 0 && volume <= 100 ) {
> + return volume;
> + }
> +
> + return 0;
Query the volumeSlider.
> +}
> +
> void MediaWidget::changeVolume(int volume)
> {
> audioOutput->setVolume(volume * qreal(0.01));
> @@ -915,6 +1014,16 @@
> mediaObject->seek(mediaObject->currentTime() + 1000 *
> longSkipDuration);
> }
> class MediaWidget : public QWidget
> {
> Q_OBJECT
> + friend class MprisPlayerObject;
> + friend class MprisTrackListObject;
> + friend class MprisDvbObject;
> + friend class MprisDisplayObject;
Do not use friend classes for this purpose because of two reasons:
- there are internal functions that _really_ shouldn't be called from
outside (e.g. volumeChanged or mutedChanged, because they don't update
ui state)
- private functions usually trust their arguments etc, but in this
context more value checking is needed for calls from outside
So I've made the required functions public and rechecked that they
work correctly in all situations.
> + bool hasMedia();
> + void setNumber(int number);
> + void changeDvbChannel(int number);
> + void changeDvbChannel(const QString &name);
> + void toggleOsd();
> + void dvbLastChannel();
> + void toggleInstantRecord();
> + int getCurrentTrack();
> + void playPause();
> + void pause();
> + void setDvbChannel(int number);
> + void setDvbChannel(const QString &name);
> + void setDvbLastChannel();
> + void fullScreen();
> + void delTrack(int position);
> + void setLoop(bool loop);
> + void setRandom(bool random);
Removed all those functions because they are a) redundant (playPause,
pause --> simplified to togglePause()) or b) don't belong here (the
dbus objects call directly the needed functions from PlaylistTab or
DvbTab).
I suggest the following steps to proceed:
a) study the comments above a bit; the patches I've commited are [1];
feel free to ask questions
b) make your svn repo clean and update it
c) continue hacking (if you need a task, you can try to create a
kdelirc profile for kaffeine, that's an xml file which provides
default associations between remote control buttons and kaffeine dbus
interface [2])
Thanks a lot,
Christoph
[1]
http://websvn.kde.org/?view=revision&revision=1055907
http://websvn.kde.org/?view=revision&revision=1056261
http://websvn.kde.org/?view=revision&revision=1056694
[2]
the resulting file would be installed to
/usr/share/kde4/apps/profiles/kaffeine.profile.xml
|