[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: kpresenter dbus slideshow control patch
From:       Thorsten Zachmann <t.zachmann () zagge ! de>
Date:       2008-10-11 5:11:28
Message-ID: 200810110711.28218.t.zachmann () zagge ! de
[Download RAW message or body]

Hello James,

> 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?

> * 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.

> 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.

>
> 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);
    }
}

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?

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.

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);
+    }
+}

+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

+++ 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.

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,

Thorsten
_______________________________________________
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]