Created attachment 126428 [details]
Patch for slideshow plugin

I have created a copy of slideshow utilities in dplugin for slideshow. It's all compiled and seems normal to me. However, I am dealing with the some problems with DInforInterface and I have not migrated SlideShowBuilder as an interface for slideshow plugin. Could you take a look at the patch and give me some advice, please?

You need a DPluginAction for infoIface() as a parent, so you don't get an interface here. We could build a connection to a DInfoInterface, but you could also just check the objectName() from the parent, whether it is "Digikam" or "Showfoto" or "Image Editor" to know if albums are possible.

However, the SlideShowBuilder definitely needs a DInfoInterface.

Instead of the QAction* you have to use DPluginAction*, then fetching the DInfoInterface works via sender().

Maik

Thanks Maik,

I will try it. 

Nghia

Nghia,

Any progress here ? Do you need help or guidance ?


Gilles

Gilles,

Actually Yes,

It's still unclear to me how the information is passed between DInfoIterfaces and what is the link of classes SlideShowBuilder and SlideShow except that they use SlideShowSettings in common.

Could you give me a guide on the order of execution in the setup and teardown routines of a slideshow, please?

Nghia

SlideshowBuilder is just a class to parse album recursively. It's only used by digiKam, not shwofoto, as this one do not support albums.

Slideshow tool can be started from digiKam, and only digiKam in recursive mode from main menu. SlideshowBuilder will parse current album content to pupolate items, and recursively, it will descend in children albums to do the same.

At end, Slideshow settings will contains all albums items parsed. Slideshow will use this settings to render items.

SlideshowBuilder is query the database to list album items. This class must be moved in DInfoIface from libs/database/utils/iface/, the derivated class of abstract DInfoInterface from libs/dplugins/iface/. DInfoIface is only used by digiKam.

So the abstract DInfoIfnterface must integrate a virtual pure method to parse recursively albums and return a QList<Url> to be generic and compatible with SlideShowSettings.

The DMetaInfoIface must implement an empty version of this new method. It's for Showfoto.

The DInfoIface must use SlideshowBuilder code to populate items using database.

Note : SlideShowBuilder class must be renamed as AlbumsParser or something like that to be more generic, as DInfoIface can be used by other plugins in the future.

Gilles

Thanks Gilles,

It has been clearer to me.

Could you point me to the function or the file where the slideshow is initiated from the main interface? Because I have only found codes in app/view/stack where some signals are used to init SlideShowBuilder, and I am not sure if it is where slideshow is really initiated.

Nghia.

Hi Nghia,

1/ If you want to uderstand how to work the plugin architecture, take a look first in the test CLI tool for generic plugins:

https://invent.kde.org/kde/digikam/-/blob/master/core/tests/dplugins/loadandrun_generic.cpp

This code is able to load plugin shared object as stand alone application and start a delegate action embeded in tool. You can test with the ported version of Slideshow to see if all run fine.

[gilles@localhost dplugins]$ ./loadandrun_generic --help
"/usr/lib64"
"/usr/lib64/qt5/libexec"
"/usr/lib64/qt5/plugins"
Usage: ./loadandrun_generic [options] +[file(s)]
Test application to run digiKam plugins as stand alone

Options:
  -h, --help        Displays this help.
  --list            List all available plugins
  -l <Plugin IID>   Unique name ID of the plugin to use
  -a <Action Name>  Plugin action name to run
  -w                Wait until plugin non-modal dialog is closed

Arguments:
  files             File(s) to open

In this code you will found all the core methods called to play with a plugin using DPlugins API. You will see that this code is the most generic as possible to be able to handle all plugins without specific code. This is the goal of the famous Abstract interface re-implemented for the host application (digiKam or Showfoto) 

2/ Some pointers from the digiKam core implementation to found all calls relevant of plugins management.

- digiKam and Showfoto main windows are derivated from this class :

https://invent.kde.org/kde/digikam/-/blob/master/core/libs/widgets/mainview/dxmlguiwindow.h

- now look the virtual pure methods which must be re-implemented in derivated classes for digiKam and Showfoto :

https://invent.kde.org/kde/digikam/-/blob/master/core/libs/widgets/mainview/dxmlguiwindow.h#L166

This one is to create instance of the plugin interface...

2 other ones are to register plugin action in menu, button context menu, etc, accordingly with the Setup plugins configuration where the user can enable or disable a tool between sessions.

https://invent.kde.org/kde/digikam/-/blob/master/core/libs/widgets/mainview/dxmlguiwindow.h#L160

3/ In digiKam, all the plugins are loaded at run time through DPluginLoader :

https://invent.kde.org/kde/digikam/-/blob/master/core/app/main/digikamapp.cpp#L134

...and unloaded in destructor of course :

https://invent.kde.org/kde/digikam/-/blob/master/core/app/main/digikamapp.cpp#L261

4/ this is the implementation to pass the plugin interface instance in digiKam. Some arguments are wrapped depending of some settings from Setup/Misc configuration tab about the images grouping feature (digiKam only).

https://invent.kde.org/kde/digikam/-/blob/master/core/app/main/digikamapp.cpp#L1071

5/ For digiKam album GUI, Slideshow is called from icon-view :

https://invent.kde.org/kde/digikam/-/blob/master/core/app/views/stack/itemiconview_slideshow.cpp

All these slots are called from the core of this class :

https://invent.kde.org/kde/digikam/-/blob/master/core/app/views/stack/itemiconview.h#L204

All these methods must be moved in the plugin implementation. They calls the famous builder to populate items from albums depending of the plugin action called by user (recursive or not). In the plugin, this kind of method must call the moved builder in the host plugin interface of course, to be independent. 

6/ Finally where is instancied the Slideshow tool instance in Album GUI version. It's simple: when builder has fully populated items from albums :

https://invent.kde.org/kde/digikam/-/blob/master/core/app/views/stack/itemiconview_slideshow.cpp#L96

Look like we pass the host plugin interface instance as argument...

7/ Don't forget the rest of digiKam SLideshow calls to migrate/port/fix with the new plugin Slideshow code. All are more simpler, as the recursive mode do not exists in these cases :

* The DK Image Editor (this is not Showfoto) :

https://invent.kde.org/kde/digikam/-/blob/master/core/utilities/imageeditor/main/imagewindow_tools.cpp

* The Light Table:

https://invent.kde.org/kde/digikam/-/blob/master/core/utilities/lighttable/lighttablewindow_tools.cpp#L56

Here codes must be factorized/moved/merged with builder and host plugin interface for digiKam...

Voilà.

Gilles

Nghia,

We are close to have a finalized patch. Please update it while this week end if possible, i would to review and include code for next 7.0.0 beta 3 planed at 16 march.

Tomorrow i teach all the day, but Sunday i will able to work on it if you complete the patch.

Note: student proposals summission will be open in middle of March. So don't waste time now to write the paper in advance and share it with google doc for the re-read stage with mentors.

Thanks in advance

Gilles

Hello Gilles,

I am finalizing it. I will deliver to you the patch tomorrow.

great thanks...

Gilles

Created attachment 126694 [details]
Replace Utilities/slideshow by dplugin slideshow

Hello,

I have finished the migration of slideshow to dplugin. The completed patch is in the attached file below. 

However, I meet a problem to update Metadata of an image from slideshow through the DInfoInterface. I haven't found the method to rewrite the change in Image metadata to the database after each change. I doubt that it exists. 

Could you take a look at the patch and help me reverify these functionalities? If they haven't existed, can I have permission to add it to the code base of DInfoInterface?

Thank you in advance.

Nghia

I take a loo while this morning and give you a feedback

Gilles

>Could you take a look at the patch and help me reverify these functionalities? >If they haven't existed, can I have permission to add it to the code base of >DInfoInterface?

Yes, typically, something is missing in digiKam interface (not the abstract interface, and not for Showfoto).

For digiKam, if connection are missing as you comment in your patch here :

/*
    //TODO: port these slot from itemiconview_tags.cpp for Digikam, Image Editor, Light Table but now Showfoto via DBInfoIface
    connect(slide, SIGNAL(signalRatingChanged(QUrl,int)),
            this, SLOT(slotRatingChanged(QUrl,int)));

    connect(slide, SIGNAL(signalColorLabelChanged(QUrl,int)),
            this, SLOT(slotColorLabelChanged(QUrl,int)));

    connect(slide, SIGNAL(signalPickLabelChanged(QUrl,int)),
            this, SLOT(slotPickLabelChanged(QUrl,int)));

    connect(slide, SIGNAL(signalToggleTag(QUrl,int)),
            this, SLOT(slotToggleTag(QUrl,int)));
*/

... connections must be established in constructor (or something like that):

https://invent.kde.org/kde/digikam/-/blob/master/core/libs/database/utils/ifaces/dbinfoiface.cpp#L281

... and signals emitted in this method:

https://invent.kde.org/kde/digikam/-/blob/master/core/libs/database/utils/ifaces/dbinfoiface.cpp#L476

Gilles

Minh,

There is a glitch in patch :

/home/gilles/Documents/GIT/7.x/core/dplugins/generic/view/slideshow/common/slideshowloader.cpp: In member function ‘void DigikamGenericSlideShowPlugin::SlideShowLoader::slotLoadNextItem()’:
/home/gilles/Documents/GIT/7.x/core/dplugins/generic/view/slideshow/common/slideshowloader.cpp:344:10: error: ‘FR’ was not declared in this scope
         {FR
          ^~
[ 69%] Building CXX object core/dplugins/generic/view/glviewer/CMakeFiles/Generic_GLViewer_Plugin.dir/glviewertexture.cpp.o
make[2]: *** [core/dplugins/generic/view/slideshow/CMakeFiles/Generic_SlideShow_Plugin.dir/build.make:76: core/dplugins/generic/view/slideshow/CMakeFiles/Generic_SlideShow_Plugin.dir/common/slideshowloader.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:9486: core/dplugins/generic/view/slideshow/CMakeFiles/Generic_SlideShow_Plugin.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

Else, i take a look in the rest of your code, an it sound like well done until now...

I wait your next patch version to review.

Best

Gilles

I am adding these updating functionalities to setItemInfo of DBInfoIface but one thing a little bit weird appears to me. It is that the method toggleTag of SlideShow is used nowhere. This method only emit a signal to inform digikam and Light table that the tag has changed. 

Could you help me identify the use of this method, please? If it has no use, can I delete it?

Nghia

Mingh,

Look well : SlideShow::toogleTag(int) function is used... at just one place : in TagsActionMngr class :

https://invent.kde.org/kde/digikam/-/blob/master/core/libs/tags/engine/tagsactionmngr.cpp#L519

What's this class exactly ? It's a manager to handle tags keyboard shortcuts. User can assign a shortcut to a tag. This permit to assign keywords to images quickly using keyboard only.

This manager is able to drive tags as said previously, but also ratings, flags, and color labels, using default shortcuts (not customizable). 

In others word, for Slideshow, when user play with shortcuts to assign a properties, and if slideshow is active, a signal is rerouted internally at the right place.

Gilles

Hello,

I have completed the plugin slideshow and here is the link to my merge request: 
https://invent.kde.org/kde/digikam/-/merge_requests/11

Thank you in advance for reviewing it for me.

Nghia

Thanks a lots Nghia,

I review your patch today for a final integration in git/master

Best

Gilles

Maik,

I applied all patches from Nghia about this entry (81 commits).

All the coding style, implementations, analysis are in top level. You can judge yourself :

https://invent.kde.org/kde/digikam/commit/8c35848a57da58c792f36f9747a6478c164a0831

I still few adjustement to do to finalize the port. It miss 2 signals / slots connections between icon-view instances and slideshow through the digiKam plugin interface, when the tool is done. I waiting a new small patch from Nghia fo that.


After this task, Nghia, will post a proposal about the DNN GoSC 2020 project for this summer.

Best
Gilles

Git commit 1ec58df1a518be6f8169039df43e65148fcc990e by Maik Qualmann.
Committed on 21/03/2020 at 21:58.
Pushed by mqualmann into branch 'master'.

implement go to the last URL from the slideshow to the icon view
FIXED-IN: 7.0.0

M  +6    -0    core/app/main/digikamapp.cpp
M  +1    -0    core/app/views/stack/itemiconview.h
M  +13   -0    core/app/views/stack/itemiconview_items.cpp
M  +4    -21   core/dplugins/generic/view/slideshow/slideshowplugin.cpp
M  +0    -2    core/dplugins/generic/view/slideshow/slideshowplugin.h
M  +2    -0    core/libs/dplugins/iface/dinfointerface.h
M  +4    -2    core/utilities/lighttable/lighttablewindow_tools.cpp

https://invent.kde.org/kde/digikam/commit/1ec58df1a518be6f8169039df43e65148fcc990e

Since the slideshow preferences have moved in 7.0.0-beta3 to within the slideshow screen itself as part of the progress indicator buttons, it is observed that the progress indicator setting is not persistent between slideshow uses, and has to be toggled each time.

You seem to have already anticipated this, because of course, if it was persistent then once the progress indicator was switched off it would be impossible to enter the dialog again without resorting to editing ~/.config/digikamrc and toggling SlideShowProgressIndicator=true

Maybe the settings should be available on a hotkey, so then persistent settings can be changed when not otherwise visible. Otherwise you have to exit the slideshow and re-enter before you can change settings.

I may be missing that the settings are already on a hotkey, and I'd be grateful to know what that is if so!

Please consider this change, as changing settings each time to remove the progress bar is a hump in the process, and a divergence from behaviour in 7.0.0-beta2 and earlier.

Hi Nghia,

If you can provide a small patch for the Andy comment #22, it will be great.

Thanks in advance

Gilles

Hello,

Yes, sure. I will try

(In reply to griffiths_andy from comment #22)
> Since the slideshow preferences have moved in 7.0.0-beta3 to within the
> slideshow screen itself as part of the progress indicator buttons, it is
> observed that the progress indicator setting is not persistent between
> slideshow uses, and has to be toggled each time.
> 
> You seem to have already anticipated this, because of course, if it was
> persistent then once the progress indicator was switched off it would be
> impossible to enter the dialog again without resorting to editing
> ~/.config/digikamrc and toggling SlideShowProgressIndicator=true

Yes, you are right. If we set to not showing this progress indicator, the only way to switch it back is to modify the config file.

> Maybe the settings should be available on a hotkey, so then persistent
> settings can be changed when not otherwise visible. Otherwise you have to
> exit the slideshow and re-enter before you can change settings.

Configure it in a hotkey is doable. However, it could be a little bit confusing for some users, since everyone tends to not looking for a hotkey to solve the problem when the progress indication disappears. Therefore, I am not sure it's a good idea to put it in a hotkey.

> I may be missing that the settings are already on a hotkey, and I'd be
> grateful to know what that is if so!
> 
> Please consider this change, as changing settings each time to remove the
> progress bar is a hump in the process, and a divergence from behaviour in
> 7.0.0-beta2 and earlier.

How about we do it otherwise, by using a hotkey to hide the progress bar? Then it will never appear again until the hotkey is repressed.

Hi Nghia,
Thanks for agreeing to have a look at my issue.

I agree, the use of a hotkey does seem like it will be a confusing thing, and not obvious unless referenced in documentation, which of course not everyone reads initially as the UI is usually quite obvious and straightforward! It was the first thought off the top of my head tbh.

I'm assuming that the plugin architecture mandates that setting manipulation should take place within the confines of the plugin dialog, hence why the settings were moved from the main config panel. Thus we have to make it work within the plugin. Of course, this whole discussion also applies to showfoto, as I assume they both use the same plugin.

> How about we do it otherwise, by using a hotkey to hide the progress bar? Then 
> it will never appear again until the hotkey is repressed.

I think this approach leaves the progress bar visible on entry to the slideshow and it remains as another action the user has to do to hide it prior to the start of the show.

Perhaps something like a hot-corner or hot-edge could be used to pop out the progress bar. So normally the progress bar can remain hidden if it's preference is set that way, but if the user hovers the mouse towards a defined edge of screen, it pops up so it can be interacted with. It would need a delay to avoid false triggering, and sufficient dwell time when the user moves the mouse away so that the user is not trying to chase it around the screen. This seems to be a workable approach, and I've certainly seen its use in other applications.

So to sum up:
- Allow the progress bar visibility setting to remain persistent
- A method to allow the progress bar to become visible on demand
- No other hidden methods, documentation, arcane knowledge required!

Thanks again,

Andy

Git commit a6dff862e40beb4460bd0bade01746fbb96601df by Maik Qualmann.
Committed on 14/04/2020 at 20:33.
Pushed by mqualmann into branch 'master'.

add the F2 key as a workaround to show the settings
The F2 key is also described on the help page with F1

M  +1    -0    core/dplugins/generic/view/slideshow/widgets/slidehelp.cpp
M  +33   -4    core/dplugins/generic/view/slideshow/widgets/slidetoolbar.cpp

https://invent.kde.org/kde/digikam/commit/a6dff862e40beb4460bd0bade01746fbb96601df

(In reply to Maik Qualmann from comment #27)
> Git commit a6dff862e40beb4460bd0bade01746fbb96601df by Maik Qualmann.
> Committed on 14/04/2020 at 20:33.
> Pushed by mqualmann into branch 'master'.
> 
> add the F2 key as a workaround to show the settings
> The F2 key is also described on the help page with F1
> 
> M  +1    -0    core/dplugins/generic/view/slideshow/widgets/slidehelp.cpp
> M  +33   -4    core/dplugins/generic/view/slideshow/widgets/slidetoolbar.cpp
> 
> https://invent.kde.org/kde/digikam/commit/
> a6dff862e40beb4460bd0bade01746fbb96601df

Thanks Maik, I'll give it a try.

Andy

F2 to show the prefs works OK. Could you now please address the persistence of the 'Show Progress Indicator' setting? That's what I originally wanted looking at so that I don't need to change settings every time I use the slideshow, and was the pre-beta3 behaviour.

Thanks..

I have already restored the setting "Show Progress Indicator" to save in the config file. Will work again in the next pre-release bundles.

Maik