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