[prev in list] [next in list] [prev in thread] [next in thread]
List: koffice-devel
Subject: Re: kpresenter dbus slideshow control patch
From: James Hogan <james () albanarts ! com>
Date: 2008-10-11 14:52:46
Message-ID: 20081011155246.wh1v4repwkk8okws () webmail ! opentransfer ! com
[Download RAW message or body]
Hi Thorsten
Thanks for looking at the patch and for the comments.
>> I've attached a patch for kpresenter based on koffice 2.0 beta 1 which
>> implements a basic dbus slideshow control interface (see
>> http://bugs.kde.org/show_bug.cgi?id=172115).
>> Its my first patch submission so if i've done anything silly or contrary to
>> the coding styles please do let me know.
>>
>> the interface (KPrViewAdaptor) has the following functionality:
>> * show "edit custom slideshows" dialog, list custom slideshows, get/set
>> active custom slideshow
>> * start, stop, navigate (prev/next step/slide, first, last) slideshows
>> * running slideshow information (is one running, current slide/step, num
>> steps in slide, number of slides, go to slide)
>
> Is there a special reason you want to have access to the current/number of
> steps in a slide?
All of the changes are made with the intention of improving the
integration of kpresenter into kworship like programs.
In this case it allows the gui to show how many steps through the
slide as a progress bar. It also helps kworship decide whether to
enable/disable the next/previous step buttons. This functionality was
available from the old DCOP interface and is useful if there are a
number of points on a slide and you don't want to accidently go onto
the next slide when clicking through them.
>> * signals for the following events:
>> * active custom slideshow is changed
>> * custom slideshows have been modified
>> * slideshow has been started/stopped
>> * slideshow slide/step has changed
>
> What are the reasons for providing this signals?
>
> I'm asking as I'm not sure if these are needed for e.g. remote
> controlling the
> presentations. Do you have some use cases for the use of those functionality.
KWorship shows a list of open presentations (in a separate change I've
also added signals for when documents are opened / closed which is
handy for keeping this list up to date). when a presentation is
selected I have a combobox of custom slideshows (which defaults to all
slides). The slides in the active custom slideshow are displayed as
thumbnails in a list view and buttons are provided to start/stop the
presentation and navigate the slides.
The selected custom slideshow in this combobox is kept in sync with
the active custom slideshow of the document using the active slideshow
changed signal. It isn't absolutely essential, but several bits of the
dbus interface access data in the active slideshow, so without this
signal i would have to ensure that the active slideshow is correct
before using these accessors.
when the user modifies the custom slideshows using the edit custom
slideshows dialog box, kworship uses the custom slideshow modified
signal to keep its list of slideshows up to date. a simple use-case is
when the user wants to add or edit a custom slideshow. they use the
edit custom slideshow to edit a slideshow or add one, and as soon as
they okay the dialog box, the list of custom slideshows in kworship
updates automatically (and if the active slideshow is changed, the
list of slide previews also update accordingly to show the new slides).
The start/stop presentation signals allow kworship to update its
interface when the slideshow is started or stopped from kpresenter (to
enable/disable widgets and make the slideshow progress bar visible).
This is mostly just to avoid inconsistent interfaces, but i can
imagine a use case where a spelling mistake in a auto looping
slideshow is noticed, so the user stops, corrects, and starts the
slideshow quickly directly from kpresenter to avoid switching focus
and would expect the kworship gui to stay up to date.
The changed slide/step signals allow KWorship to update its gui
(change progress through slideshow, highlight current slide in list
and scroll down to it if necessary, and also update preview of current
slide in KWorship's "live preview" widget). This is particularly
important when the slideshow is timed (e.g. for notices at the
beginning of a church service which might loop a slideshow on 10
seconds per slide).
>> This has meant adding some functionality to KPrAnimationDirector,
>> KPrDocument, KPrView and KPrViewModePresentation.
>>
>> I'm now going to start work on making slide information available (title,
>> outline, notes) and porting the kpresenter 1.6 exportPage function which
>> allows a preview of a slide to be saved to a file.
>
> For exporting the page you might have a look at KoPAPage::thumbnail. That is
> what is used at the moment for creating a thumbnail. It is not yet perfect
> for kpresenters needs as it also shows the placeholder shapes and cannot
> produce a certain step of a slide.
thanks. yeh KoPAPage::thumbnail is what i have used (i've implemented
a similar interface to the DCOP view::exportPage function and it works
just right now)
>> All comments + suggestions are welcome.
>
>
> void KPrDocument::setActiveCustomSlideShow( const QString &customSlideShow )
> {
> + bool changed = (customSlideShow != m_activeCustomSlideShow);
> m_activeCustomSlideShow = customSlideShow;
> + if (changed)
> + {
> + activeCustomSlideShowChanged(customSlideShow);
> + }
> }
>
> how about changing that to:
>
> void KPrDocument::setActiveCustomSlideShow( const QString &customSlideShow )
> {
> if (customSlideShow != m_activeCustomSlideShow) {
> m_activeCustomSlideShow = customSlideShow;
> activeCustomSlideShowChanged(customSlideShow);
> }
> }
haha, it must have been late at night when I did that.
> void KPrViewAdaptor::editCustomSlideShowsDialog()
>
> Is that needed? It only opens the dialog but you still need to do all the
> other stuff by hand. So it is not really usefull for remote controlling it.
> Or do I miss something here?
The reason I added that is to have a button on the KWorship GUI next
to the custom slideshows combo box to edit them. Its not ideal as it
doesn't bring itself into focus properly, and its certainly not
essential. I'm happy to remove if you like.
> I also think if possible it should be avoided to use the KPrAnimationDirector
> directly. That exposes the internals quite a bit which I think should be
> avoided. But lets first see what are use cases I asked above.
I'll alter to make the adaptor class thinner and move any
functionality in it to the view.
> e.g. use
>
> KPrViewModePresentation::navigate( KPrAnimationDirector::Navigation
> navigation )
>
> instead of
>
> +void KPrViewAdaptor::screenPrev()
> +{
> + if (0 != m_view->presentationMode()->animationDirector())
> + {
> +
> m_view->presentationMode()->animationDirector()->navigate(KPrAnimationDirector::PreviousStep);
> + }
> +}
Ah, i didn't notice KPrViewModePresentation::navigate. that would
indeed be better.
> +bool KPrViewAdaptor::isPresRunning() const
> +{
> + return (m_view->presentationMode() == m_view->viewMode());
> +}
>
> How about adding that functionality to KPrView and use the method that method
> instead. With that the following could also use the new function
>
> + if (0 != m_view->presentationMode()->animationDirector())
> Here are some style issues that should be fixed
>
> + if (name == "" || customSlideShows().contains(name))
> + {
> + m_view->kprDocument()->setActiveCustomSlideShow(name);
> + }
>
> The opening { should go to the end of the line with the if/for/while
ok thanks, i can update my styling easily enough.
> +++ b/kpresenter/part/KPrDocument.h
> @@ -117,6 +117,21 @@ public:
> */
> void setActiveCustomSlideShow( const QString &customSlideShow );
>
> +Q_SIGNALS:
>
> We are using signals: instead of Q_SIGNALS.
ok, I saw Q_SIGNALS elsewhere and wasn't really sure if there was a
difference or if it was purely a style issue.
> If you have questions please send me a mail.
>
> Sorry again that it took so long until I had some time to look at your patch.
>
> Have a nice weekend,
Thanks. No problem about the time. Having just moved back to
university i'm without broadband for a couple of weeks anyway.
I'll let you know soon about the other bits that I've added after this
patch, so i can get some feedback.
Cheers
James
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic