Bug 286018

Benjamin, any news about this patch ?

Gilles Caulier

I do not have time at the moment (mid-term exams). I will review it during December.

When this patch is used with the panorama tool on Windows there are some problems:

1. panorama tool doesn't seem to search properly on its own before it asks for the location of the appropriate executable (perhaps due to 2. below)

2. When asking the user to browse for the appropriate executable the windows extension '.exe' is missing from the application name so no applications show up when browsing (unless the user knows how to get around this limitation). For instance it will try to browse for 'pto2mk' instead of 'pto2mk.exe'

3. After locating one of the required executables, the panorama tool asks the user to browse for the remaining missing executables instead of finding it automatically based on the same folder of the first found executable (perhaps due to 2.)

4. If the user does not have one of the executables then the panorama tool asks for the remaining tools to be located but should instead just display the 'install hugin' message directly

5. After the 'install hugin' message is shown once, running the panorama tool again does not prompt to browse for any hugin executables but instead presents the 'install hugin' message unless digiKam is restarted. Therefore the user who downloaded and installed hugin would have to restart digiKam before they could use the panorama tool after receiving the 'install hugin' message

thanks for the feedback, its good to get another prospective. i'll try to improve on those points. Other that those problems, were you able to get the pano tool to work on windows?

(In reply to comment #4)
> thanks for the feedback, its good to get another prospective. i'll try to
> improve on those points. Other that those problems, were you able to get the
> pano tool to work on windows?

Not quite. There was a problem that crashed pto2mk (not your fault) that was pre-existing I had to fix first. Also, make doesn't run properly because your editing of the makefile doesn't work on Windows. Instead of editing the makefile, can't you just set the PATH environment variable to contain the path to the Hugin executables?

Likewise, why can't you do the same thing once you have found one of the executables, instead of asking where all the others are, store the path from the first and use that to find all other executables. Would also eliminate the possibility of using executables from different installs (unlikely the user would do this, but still a possibility).

Before you suggest a new patch, please update to latest source since Benjamin also made some code changes.

On Nov 30, 2011, at 7:31 AM, Ananta Palani wrote:

> https://bugs.kde.org/show_bug.cgi?id=286018
> 
> 
> 
> 
> 
> --- Comment #5 from Ananta Palani <anantapalani gmail com>  2011-11-30 12:31:18 ---
> (In reply to comment #4)

> can't you just set the PATH environment variable to contain the path
> to the Hugin executables?
> 
i had troubles with setting the PATH in the QProcess. I don't recall exactly what the problems were; this is way i decided to edit the makefile. When i have a revisited patch that applies cleanly to the new codebase and address your other complaints, i'll take another look at this problem of PATH vs. edit Makefile.


> Likewise, why can't you do the same thing once you have found one of the
> executables, instead of asking where all the others are, store the path from
> the first and use that to find all other executables. 
> 
Is this the same point (#3) you made in your previous post?

> Before you suggest a new patch, please update to latest source since Benjamin
> also made some code changes.
> 
i am working on a new submission of the patch to apply cleaner to the recently changed files and to address the problems you're seeing. i guess i wasn't clear about that in my last post. 

Note to Benjamin: this patch needs some work, please wait until i have resubmitted that patch to address these problems before you spend time on reviewing it.

Created attachment 66523 [details]
Take 2, Pano/Expo blend patch for OS X (and maybe Windows)

Ok. i have worked on a second revision of this patch. To be honest, this is far larger of a code change than i really want; this is mostly due to some changes in the gui. With this patch the first page of the Wizard Dialog displays a list of binaries that are necessary to do these processes (i'll attach a screen shot to give you an idea). Once potential improvement i can think of, is that instead of listing all the binaries necessary, i might want to simplify it to just have to use navigate to the Hugin package. 

with regards to Ananta's comments, i think points 2 and 3 are addressed, points 4 and 5 no longer pertain to the new interface since.

Created attachment 66524 [details]
Screencap of new dialog.

Benjamin, Ananta,

Do you see last comments from Brad ?

Gilles Caulier

Created attachment 67090 [details]
Take 3, pano/expo blend patch for os x (and maybe windows)

now that i have a recent working version of digikam, i can test my patches agains the last code. This patch should apply clearly to the latest repo. (of course i thought that the last patch would apply cleanly.)

Git commit 107585f1e1c91176a5ef50d51dbdd4a4097d1643 by Benjamin Girault.
Committed on 08/01/2012 at 20:07.
Pushed by girault into branch 'master'.

WIP with Brad's patch

 - binaryiface: cleanup
 - binarysearch: new widget to setup executable paths (unified solution
   for all plugins, without any tie to Hugin)

M  +1    -0    common/libkipiplugins/CMakeLists.txt
M  +48   -53   common/libkipiplugins/tools/binaryiface.cpp
M  +61   -61   common/libkipiplugins/tools/binaryiface.h
A  +125  -0    common/libkipiplugins/widgets/binarysearch.cpp     [License: GPL (v2+)]
A  +71   -0    common/libkipiplugins/widgets/binarysearch.h     [License: GPL (v2+)]

http://commits.kde.org/kipi-plugins/107585f1e1c91176a5ef50d51dbdd4a4097d1643

Git commit 0fa4440fb1129ee306e58779bbab1d3fe2a15e84 by Benjamin Girault.
Committed on 05/01/2012 at 20:12.
Pushed by girault into branch 'master'.

Patch from brad -- Part 1

Introduces finer (multi platform version) binaries detection for
kipi-plugins.

M  +177  -1    common/libkipiplugins/tools/binaryiface.cpp
M  +68   -5    common/libkipiplugins/tools/binaryiface.h
M  +10   -17   expoblending/manager/alignbinary.cpp
M  +2    -2    expoblending/manager/alignbinary.h
M  +11   -17   expoblending/manager/enfusebinary.cpp
M  +2    -2    expoblending/manager/enfusebinary.h
M  +12   -18   panorama/manager/autooptimiserbinary.cpp
M  +3    -2    panorama/manager/autooptimiserbinary.h
M  +9    -17   panorama/manager/cpcleanbinary.cpp
M  +2    -2    panorama/manager/cpcleanbinary.h
M  +26   -22   panorama/manager/cpfindbinary.cpp
M  +2    -6    panorama/manager/cpfindbinary.h
M  +9    -17   panorama/manager/enblendbinary.cpp
M  +4    -2    panorama/manager/enblendbinary.h
M  +9    -17   panorama/manager/makebinary.cpp
M  +2    -2    panorama/manager/makebinary.h
M  +9    -17   panorama/manager/nonabinary.cpp
M  +2    -2    panorama/manager/nonabinary.h
M  +11   -12   panorama/manager/pto2mkbinary.cpp
M  +2    -2    panorama/manager/pto2mkbinary.h

http://commits.kde.org/kipi-plugins/0fa4440fb1129ee306e58779bbab1d3fe2a15e84

Git commit a325500c7484c83ea71d335639e6d22419856b69 by Benjamin Girault.
Committed on 09/01/2012 at 22:20.
Pushed by girault into branch 'master'.

Cleanup after Brad's patch reviewing

 - Regression bugfix (in cpfindbinary)
 - Code cleanup

M  +1    -1    expoblending/importwizard/importwizarddlg.h
M  +1    -1    expoblending/manager/alignbinary.cpp
M  +3    -1    expoblending/manager/alignbinary.h
M  +1    -1    expoblending/manager/enfusebinary.cpp
M  +1    -1    expoblending/manager/enfusebinary.h
M  +1    -1    panorama/importwizard/optimizepage.cpp
M  +1    -1    panorama/importwizard/preprocessingpage.cpp
M  +1    -1    panorama/manager/autooptimiserbinary.cpp
M  +1    -1    panorama/manager/autooptimiserbinary.h
M  +1    -1    panorama/manager/cpcleanbinary.cpp
M  +1    -1    panorama/manager/cpcleanbinary.h
M  +3    -3    panorama/manager/cpfindbinary.cpp
M  +1    -1    panorama/manager/cpfindbinary.h
M  +1    -1    panorama/manager/enblendbinary.cpp
M  +1    -1    panorama/manager/enblendbinary.h
M  +1    -1    panorama/manager/makebinary.cpp
M  +1    -1    panorama/manager/makebinary.h
M  +1    -1    panorama/manager/nonabinary.cpp
M  +1    -1    panorama/manager/nonabinary.h
M  +1    -8    panorama/manager/pto2mkbinary.cpp
M  +1    -1    panorama/manager/pto2mkbinary.h

http://commits.kde.org/kipi-plugins/a325500c7484c83ea71d335639e6d22419856b69

The patch is reviewed and integrated into the repository. It works (at least) on linux. Can somebody confirm that it works on MacOS and Windows?

Brad: I like your solution to the issue, and only made some changes to make it more general and straightforward for the future. I only have one question: is there a reason why you kept the "virtual" keyword in front of every "parseHeader" function of BinaryIFace children?

On Jan 9, 2012, at 4:46 PM, <benjamin.girault@gmail.com> <benjamin.girault@gmail.com> wrote:

> https://bugs.kde.org/show_bug.cgi?id=286018
> 
> 
> benjamin.girault@gmail.com changed:
> 
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>             Status|UNCONFIRMED                 |ASSIGNED
>     Ever Confirmed|0                           |1
> 
> 
> 
> 
> --- Comment #15 from  <benjamin girault gmail com>  2012-01-09 21:46:57 ---
> The patch is reviewed and integrated into the repository. It works (at least)
> on linux. Can somebody confirm that it works on MacOS and Windows?
> 
> Brad: I like your solution to the issue, and only made some changes to make it
> more general and straightforward for the future.

Great! i'm glad i could finally get a patch that would apply, it only took three tries.

> I only have one question: is
> there a reason why you kept the "virtual" keyword in front of every
> "parseHeader" function of BinaryIFace children?
> 
No good reason, feel free to eliminate that. 


I have tested the new code and noticed a couple of things:

 1) suppose multiple binaries are not found immediately, then when use navigates to one binary, the will have to navigate to the next binary even if both binaries exists in the same directory, which is usually the case for the hugin binaries. In my patch i had come connections in manager.{cpp,h} which propagated a signal though the classes to add a directory to the search path list. I can submit another patch off the latest codebase for this.

 2) i appears that if the development version of a binary is being used that one cannot proceed. for some reason my cpfind binary is flagged as development version. Looking at it more closely, any version of cpfind that contains the ThreadQueue text will get flagged as development version since once through the foreach-loop in cpfindbinary.cpp:64 will set m_developmentVersion to true. If the binary contains the ThreadQueue text will go through the loop at least once.

 3) if binaries not found, the window becomes very wide --- too wide for my laptop to handle. I think that's because of the warning message displayed.

 4) the 'download' link to binaries not found appears below the groupbox, i assume it is meant to be inline within the rows in the groupbox.

Lastly, i have no idea how this works (or doesn't work) under windows. i do not have a windows machine to test on.

(In reply to comment #16)
> Lastly, i have no idea how this works (or doesn't work) under windows. i do not
> have a windows machine to test on.

I'll build and test it on Windows after all parties are satisfied with the behavior on Linux/OS X.

[Here is my answer to Brad]

1) To be honest, I haven't tested the case where a binary is not found, mostly because I have them and to publish the code quicker. I'll dig into this problem, this week or at the coding sprint in Genoa this week-end (I move the thing your talking about from both managers to a new widget). Hopefully it is just a mater of some connects missing.

2) The fact that if cpfind print some stuff about ZThread is detected as a development version is the normal behavior (because this shouldn't appear). However, the plugin should proceed. I'll look into it.

3) I need to try that to see what you mean.

4) Right, it should be inlined. My guess is that the window is not large enough to fit the text on the same line. That's another thing I have to try. Note that I may remove those texts to something smarter. I'm not sure what at the moment.

On Jan 10, 2012, at 6:23 AM, <benjamin.girault@gmail.com> wrote:

> https://bugs.kde.org/show_bug.cgi?id=286018
> 
> 
> 
> 
> 
> --- Comment #18 from  <benjamin girault gmail com>  2012-01-10 11:23:45 ---

Anata: sounds fair enough.


> [Here is my answer to Brad]
> 
> 1) To be honest, I haven't tested the case where a binary is not found, mostly
> because I have them and to publish the code quicker. I'll dig into this
> problem, this week or at the coding sprint in Genoa this week-end (I move the
> thing your talking about from both managers to a new widget). Hopefully it is
> just a mater of some connects missing.
> 
i have a patch on my system. i can make a diff if you want.

> 2) The fact that if cpfind print some stuff about ZThread is detected as a
> development version is the normal behavior (because this shouldn't appear).
> However, the plugin should proceed. I'll look into it.
> 
actually ... i think this bug is caused by missing connections from the above point. i had a signal in manager.cpp called signalAllBinariesFound which was triggered to enable the 'next' button when all the binaries were found.

i'll supply a screencap. you'll see the dev version of cpfind was found, but i cannot proceed. Also, perhaps a quick tooltip or text in the version column of the window that this version is a development version, otherwise i can see a user being very puzzled by the different icon -- i should have thought of this when i submitted the patch.

> 3) I need to try that to see what you mean.
> 
> 4) Right, it should be inlined. My guess is that the window is not large enough
> to fit the text on the same line. That's another thing I have to try. Note that
> I may remove those texts to something smarter. I'm not sure what at the moment.

see screencap -- picture is worth a thousand words.

Created attachment 67654 [details]
pano dialog

Very wide window, download links not inline in groupbox, also can no proceed despite all binaries found.

Git commit af58051d7749d990d964ece2702631e01609a81c by Benjamin Girault.
Committed on 11/01/2012 at 02:14.
Pushed by girault into branch 'master'.

Bugfix on panorama

 - Address partly 2) in Brad's comment #19 from bug #286018: adding
   tooltips, and fixing connections
 - Commenting some useless stuff in Panorama

M  +3    -0    common/libkipiplugins/tools/binaryiface.cpp
M  +2    -2    common/libkipiplugins/widgets/binarysearch.cpp
M  +2    -2    panorama/importwizard/importwizarddlg.cpp

http://commits.kde.org/kipi-plugins/af58051d7749d990d964ece2702631e01609a81c

Git commit 52ffa9d3bba25ac9a4426870cffa71436b1c50e3 by Benjamin Girault.
Committed on 11/01/2012 at 01:59.
Pushed by girault into branch 'master'.

Bugfixes in binaryiface / binarysearch

 - From Brad's comment #16 in bug #286018, issues 1), 3) and 4) are solved.
 - Also, a small optimization is introduced in binaryiface (to not recheck
   a directory already added to the plugin path)

M  +12   -7    common/libkipiplugins/tools/binaryiface.cpp
M  +14   -17   common/libkipiplugins/widgets/binarysearch.cpp

http://commits.kde.org/kipi-plugins/52ffa9d3bba25ac9a4426870cffa71436b1c50e3

[Brad]: most of the issues you found should be solved now. There only the problem with a development version that prevent from proceeding to the next page in the assistant that I do not understand. Please tell me if this bug is still there.

On Jan 10, 2012, at 8:18 PM, <benjamin.girault@gmail.com> wrote:

> https://bugs.kde.org/show_bug.cgi?id=286018
> 
> 
> 
> 
> 
> --- Comment #23 from  <benjamin girault gmail com>  2012-01-11 01:18:35 ---
> [Brad]: most of the issues you found should be solved now. There only the
> problem with a development version that prevent from proceeding to the next
> page in the assistant that I do not understand. Please tell me if this bug is
> still there.
> 
Just compiled and tested and it looks great! well done! i like the new layout with the icons on the left, uses left horizontal space. i think the development version issue was related some missing connections related the adding to the search paths. It all appears to work now. I'd like to see if this can be run on windows.



> -- 
> Configure bugmail: https://bugs.kde.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.

(In reply to comment #24)
> Just compiled and tested and it looks great! well done! i like the new layout
> with the icons on the left, uses left horizontal space.

It was not easy to get those icons on the left, like that. I'm glad you like it.

> i think the development
> version issue was related some missing connections related the adding to the
> search paths. It all appears to work now. I'd like to see if this can be run on
> windows.

Great!

(In reply to comment #24)
> Just compiled and tested and it looks great! well done! i like the new layout
> with the icons on the left, uses left horizontal space. i think the development
> version issue was related some missing connections related the adding to the
> search paths. It all appears to work now. I'd like to see if this can be run on
> windows.

Compiling source now, I'll let you know how it works on Windows.

(In reply to comment #24)
> Just compiled and tested and it looks great! well done! i like the new layout
> with the icons on the left, uses left horizontal space. i think the development
> version issue was related some missing connections related the adding to the
> search paths. It all appears to work now. I'd like to see if this can be run on
> windows.

There still seems to be some problems. Only one of them is a blocker, but the other two are import usability considerations.



However, if hugin cannot be autodetected or if hugin was previously found and now no longer found (due to hugin being moved/hugin uninstalled/etc):
1. Have to browse for each executable separately. 'Find'ing one application does not try auto-find the remainder from the same 'bin' directory.

Solution: have remainder of executables auto-detected in same directory (re-run detection that is done on plugin start-up using new path?)

2. After 'find'ing all executables (all icons at left turn green) the buttons on the bottom do not activate. Only 'cancel' can be clicked, which causes digiKam to forget the new hugin executable locations.

Solution: enable 'next' and 'finish' (?) buttons after all executables are found.

If hugin is autodetected, works great except:
3. No option to change the hugin executables

Solution: Alter 'find' button text to something like 'change' if executable is known.

Is there any reason why each executable is identified separately anyway? It would be safest to assume that all hugin executables are only compatible with their packaged versions, so why would I want to be able to browse for each executable separately when they could possibly be from different versions?

Instead, why don't remove the 'find' buttons for each app and instead simply have the path to hugin executables listed above all the apps (or a message saying hugin could not be found if path is unknown), and a button to browse for the location where hugin is installed? After browsing for this location, run the executable auto-detect based on the new path and populate your executable info table. Just seems redundant to have multiple 'find' buttons and 'download' links. Just a thought.

Sorry for the weird wording on the paragraph that started out 'however', item 3 used to be before it.

Also, nice work on the patch, its looking very good!

(In reply to comment #27)
> 1. Have to browse for each executable separately. 'Find'ing one application
> does not try auto-find the remainder from the same 'bin' directory.
> 
> 2. After 'find'ing all executables (all icons at left turn green) the buttons
> on the bottom do not activate. Only 'cancel' can be clicked, which causes
> digiKam to forget the new hugin executable locations.

Are you sure you have the latest code from the repository? Those two should be corrected in my latest commits (and after testing again, it is still working on Linux).

> 3. No option to change the hugin executables
> 
> Solution: Alter 'find' button text to something like 'change' if executable is
> known.

Good point. I'll change that.

> Is there any reason why each executable is identified separately anyway? It
> would be safest to assume that all hugin executables are only compatible with
> their packaged versions, so why would I want to be able to browse for each
> executable separately when they could possibly be from different versions?

Actually, one could use Hugin's binaries from different version of Hugin in the plugin, it doesn't matter (those binaries "communicate" through pto files, i.e. Hugin project files whose syntax hasn't changed for a while).

> Instead, why don't remove the 'find' buttons for each app and instead simply
> have the path to hugin executables listed above all the apps (or a message
> saying hugin could not be found if path is unknown), and a button to browse for
> the location where hugin is installed? After browsing for this location, run
> the executable auto-detect based on the new path and populate your executable
> info table. Just seems redundant to have multiple 'find' buttons and 'download'
> links. Just a thought.

One reason would how to handle the case where the binaries are in different location. In that case, writing the GUI part would be a nightmare. Anyway, I intend to reduce the number of necessary binaries during the Coding Sprint to keep it to a minimum, and use directly Hugin's API.

(In reply to comment #29)
> Are you sure you have the latest code from the repository? Those two should be
> corrected in my latest commits (and after testing again, it is still working on
> Linux).

Yes. Just pulled and there were no changes to what I built. Were those changes not pushed on your end?

> Actually, one could use Hugin's binaries from different version of Hugin in the
> plugin, it doesn't matter (those binaries "communicate" through pto files, i.e.
> Hugin project files whose syntax hasn't changed for a while).
> 
> One reason would how to handle the case where the binaries are in different
> location. In that case, writing the GUI part would be a nightmare. Anyway, I
> intend to reduce the number of necessary binaries during the Coding Sprint to
> keep it to a minimum, and use directly Hugin's API.

I'm not sure I see why they would ever be in different locations, but if the plugin does try to auto-find all remaining executables after finding a single executable then this isn't really an issue anyway. If you add the ability to have a 'change' button, make sure it doesn't auto-search for executables again (unless you want that!)

I forgot to mention, the default size of the plugin window is really large and may be too large for some netbooks. Some users have complained in the past about this. Does the plugin make sure that its default window size does not exceed the user's display dimensions by default? I realize it is resizeable, but I just wanted to make sure it isn't exceeded by default.

On Jan 11, 2012, at 3:00 PM, Ananta Palani wrote:

> https://bugs.kde.org/show_bug.cgi?id=286018
> 
> 
> However, if hugin cannot be autodetected or if hugin was previously found and
> now no longer found (due to hugin being moved/hugin uninstalled/etc):
> 1. Have to browse for each executable separately. 'Find'ing one application
> does not try auto-find the remainder from the same 'bin' directory.
> 
> Solution: have remainder of executables auto-detected in same directory (re-run
> detection that is done on plugin start-up using new path?)
> 
> 2. After 'find'ing all executables (all icons at left turn green) the buttons
> on the bottom do not activate. Only 'cancel' can be clicked, which causes
> digiKam to forget the new hugin executable locations.
> 
> Solution: enable 'next' and 'finish' (?) buttons after all executables are
> found.
> 
interesting ... i had both of these problems that i mentioned in comment:#16. However after Benjamin submitted more code -- yesterday i believe -- it seemed to work. I just double checked to make sure i was checking out a clean version of the git repo, recompiled and it still works on the OS X end. I'm not sure what could be wrong on windows.

> Instead, why don't remove the 'find' buttons for each app and instead simply
> have the path to hugin executables listed above all the apps (or a message
> saying hugin could not be found if path is unknown), and a button to browse for
> the location where hugin is installed? After browsing for this location, run
> the executable auto-detect based on the new path and populate your executable
> info table. Just seems redundant to have multiple 'find' buttons and 'download'
> links. Just a thought.
> 
I had mentioned something similar in comment:#8. I had kind of abandoned that idea because of the difference in linux binaries layout and, say OS X's and windows. On OS X, if you install Hugin from hugin's website, all the binaries  are in /Applications/Hugin/HuginTools (in fact i had that hard coded in my patch since those paths remain reasonable consistent). I assume it is similar for windows, right? But on linux (i'm familiar with ubuntu), things are different. How would you have the user to navigate to Hugin on linux? There is a /usr/bin/hugin -- but that's part of the 'hugin' package, whereas the other tools are part of the hugin-tools package. I suppose that this may be a degenerate case since those binaries will be picked up since they're in the user's path; if that is the case then we can probably code something to simply navigate to the Hugin package for non-linux OS's -- i do like that solution better, seems simpler. Then, perhaps we could have an 'advanced' part of the window/settings where a user could navigate to the individual binaries -- that might address benjamin's concern about having hugin binaries installed in multiple locations. 

But ... i think this might just wait until the above three points you made are 
addressed. Although i'd like a solution where you just navigate to where hugin 
is installed for OS X/Windows.

> -- 
> Configure bugmail: https://bugs.kde.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.

Git commit 3ae22b8db12062385d5f3bce065e46b52f702b27 by Benjamin Girault.
Committed on 13/01/2012 at 17:23.
Pushed by girault into branch 'master'.

Binaries Detection code clean up

 - Corrected expected behavior
 - Code cleanup (factorization of code)
 - Updated Panorama and Expoblending codes

M  +81   -155  common/libkipiplugins/tools/binaryiface.cpp
M  +46   -46   common/libkipiplugins/tools/binaryiface.h
M  +85   -27   common/libkipiplugins/widgets/binarysearch.cpp
M  +13   -3    common/libkipiplugins/widgets/binarysearch.h
M  +8    -4    expoblending/importwizard/intropage.cpp
M  +0    -46   expoblending/manager/alignbinary.cpp
M  +6    -9    expoblending/manager/alignbinary.h
M  +6    -39   expoblending/manager/enfusebinary.cpp
M  +9    -6    expoblending/manager/enfusebinary.h
M  +0    -6    panorama/CMakeLists.txt
M  +5    -7    panorama/importwizard/intropage.cpp
M  +0    -44   panorama/manager/autooptimiserbinary.cpp
M  +5    -8    panorama/manager/autooptimiserbinary.h
M  +0    -44   panorama/manager/cpcleanbinary.cpp
M  +5    -8    panorama/manager/cpcleanbinary.h
M  +7    -35   panorama/manager/cpfindbinary.cpp
M  +8    -5    panorama/manager/cpfindbinary.h
M  +0    -43   panorama/manager/enblendbinary.cpp
M  +6    -9    panorama/manager/enblendbinary.h
M  +0    -42   panorama/manager/makebinary.cpp
M  +6    -7    panorama/manager/makebinary.h
M  +25   -25   panorama/manager/manager.cpp
M  +1    -1    panorama/manager/manager.h
M  +0    -44   panorama/manager/nonabinary.cpp
M  +6    -7    panorama/manager/nonabinary.h
M  +0    -44   panorama/manager/pto2mkbinary.cpp
M  +6    -7    panorama/manager/pto2mkbinary.h

http://commits.kde.org/kipi-plugins/3ae22b8db12062385d5f3bce065e46b52f702b27

Git commit 9fe934d020ae70908a80c19ad7c5754caada7b19 by Benjamin Girault.
Committed on 13/01/2012 at 17:34.
Pushed by girault into branch 'master'.

Binary Detection Cleanup pass 2

Deletion of useless files

M  +0    -1    expoblending/CMakeLists.txt
D  +0    -37   expoblending/manager/alignbinary.cpp
D  +0    -39   panorama/manager/autooptimiserbinary.cpp
D  +0    -37   panorama/manager/cpcleanbinary.cpp
D  +0    -37   panorama/manager/enblendbinary.cpp
D  +0    -37   panorama/manager/makebinary.cpp
D  +0    -37   panorama/manager/nonabinary.cpp
D  +0    -37   panorama/manager/pto2mkbinary.cpp

http://commits.kde.org/kipi-plugins/9fe934d020ae70908a80c19ad7c5754caada7b19

I have huge changes to the code for binary detection. Can you please test it on both MacOS and Windows? Hopefully it will work (it does at least on Linux)...

> 
> --- Comment #35 from  <benjamin girault gmail com>  2012-01-13 16:36:36 ---
> I have huge changes to the code for binary detection. Can you please test it on
> both MacOS and Windows? Hopefully it will work (it does at least on Linux)...
> 

just checked on OS X ... seems to work just fine, except one small detail (which i didn't check before). When i click on the download link, it doesn't open. I think you need the following lines for that to work,
    labelName->setTextInteractionFlags(Qt::LinksAccessibleByMouse);
    labelName->setOpenExternalLinks(true);


btw, i love the addition of this code. I was moving around my tools directory, and was stunned when it worked automatically :) ... 

#ifdef Q_WS_MAC
    d->binariesWidget->addDirectory("/Applications/Hugin/HuginTools");
#endif

i suggest that you add something similar for windows. Perhaps Ananta can tell us where the windows install path is.

awesome! i hope we can get this working on windows to make this plugin completely cross platform. I'm very happy with how things are coming together.

Git commit 855c95738dcdabcf718f685de621dd159901d8d4 by Benjamin Girault.
Committed on 13/01/2012 at 18:12.
Pushed by girault into branch 'master'.

Bugfix download link in binary detection

Thanks to brad for the report

M  +4    -1    common/libkipiplugins/widgets/binarysearch.cpp

http://commits.kde.org/kipi-plugins/855c95738dcdabcf718f685de621dd159901d8d4

(In reply to comment #36)
> just checked on OS X ... seems to work just fine, except one small detail
> (which i didn't check before). When i click on the download link, it doesn't
> open. I think you need the following lines for that to work,
>     labelName->setTextInteractionFlags(Qt::LinksAccessibleByMouse);
>     labelName->setOpenExternalLinks(true);

Done. Thank you for noticing.

(In reply to comment #31)
> I forgot to mention, the default size of the plugin window is really large and
> may be too large for some netbooks. Some users have complained in the past
> about this. Does the plugin make sure that its default window size does not
> exceed the user's display dimensions by default? I realize it is resizeable,
> but I just wanted to make sure it isn't exceeded by default.

Good point. I've push a patch on the repository to ensure that the size of the assistant is not too wide.

Also, can you confirm that the binary detection works now on Windows?

(In reply to comment #39)
Finally had time to recompile everything, sorry for the delay. It looks like the fixes you made mostly work. Here are the problems I am seeing now:

1. pto2mk.exe crashes when detected (or after detection and a restart of digikam, when panorama creator wizard is first opened)

2. clicking 'change' button browses digikam's working directory, not the directory of the given app

3. relaunch of panorama wizard within a given digikam session does not re-check existance of hugin apps (only after digikam is restarted and the panorama creator reopened does it detect moved or missing apps), so if the hugin is installed/uninstalled/moved while digikam is opened then the panorama app may not notice the change.

4. an 'Error - Preview cannot be processed.' occurs at the 'Preview and Post-Processing' page. This did not happen with code I pulled on 2012-01-11. Could be related to problem 1?

As for the suggestion to add default hugin locations for Windows, is there any problem if two directories are added? If not, then these would be the two default installation locations depending on the 32/64-bit builds:

#ifdef Q_WS_WIN
    d->binariesWidget->addDirectory("C:/Program Files/Hugin/bin");
    d->binariesWidget->addDirectory("C:/Program Files (x86)/Hugin/bin");
#endif

(In reply to comment #40)
> 1. pto2mk.exe crashes when detected (or after detection and a restart of
> digikam, when panorama creator wizard is first opened)

I should elaborate that this crash happens no matter if I chose to find pto2mk.exe directly, or if I find another executable and pto2mk.exe is auto-detected in the same directory.

(In reply to comment #40)
> 1. pto2mk.exe crashes when detected (or after detection and a restart of
> digikam, when panorama creator wizard is first opened)

Also, this did not happen with code I pulled on 2012-01-11.

Thanks for the feedback, I really appreciate it.

(In reply to comment #40)
> 1. pto2mk.exe crashes when detected (or after detection and a restart of
> digikam, when panorama creator wizard is first opened)

You are right, I changed something and introduced somehow a bug that is corrected now (I gave "-V" instead of "-h" to pto2mk for testing its availability). What do you mean by "pto2mk.exe crashes"? is it the executable itself, or also the plugin (and maybe digikam)?

> 2. clicking 'change' button browses digikam's working directory, not the
> directory of the given app

Done.

> 3. relaunch of panorama wizard within a given digikam session does not re-check
> existance of hugin apps (only after digikam is restarted and the panorama
> creator reopened does it detect moved or missing apps), so if the hugin is
> installed/uninstalled/moved while digikam is opened then the panorama app may
> not notice the change.

Good point (I didn't thought of that because I'm using the standalone version of the plugin for testing). It should be working now.

> 4. an 'Error - Preview cannot be processed.' occurs at the 'Preview and
> Post-Processing' page. This did not happen with code I pulled on 2012-01-11.
> Could be related to problem 1?

Is this error still happening with the latest commits?

> As for the suggestion to add default hugin locations for Windows, is there any
> problem if two directories are added? If not, then these would be the two
> default installation locations depending on the 32/64-bit builds:
> 
> #ifdef Q_WS_WIN
>     d->binariesWidget->addDirectory("C:/Program Files/Hugin/bin");
>     d->binariesWidget->addDirectory("C:/Program Files (x86)/Hugin/bin");
> #endif

Done.

(In reply to comment #43)
> Thanks for the feedback, I really appreciate it.

You're welcome, thanks for writing the plug-in :)

> (In reply to comment #40)
> > 1. pto2mk.exe crashes when detected (or after detection and a restart of
> > digikam, when panorama creator wizard is first opened)
> 
> You are right, I changed something and introduced somehow a bug that is
> corrected now (I gave "-V" instead of "-h" to pto2mk for testing its
> availability). What do you mean by "pto2mk.exe crashes"? is it the executable
> itself, or also the plugin (and maybe digikam)?

Doesn't crash anymore (pto2mk.exe was crashing)

> > 2. clicking 'change' button browses digikam's working directory, not the
> > directory of the given app
> 
> Done.

There were some problems with how this was implemented. I committed some changes so that this launches properly on Mac/Windows again. Basically KUrl(cstr) behaves differently from KUrl(QString) so I made simply made the paths QStrings before launching. With the former method, the native browser is not launched, but instead Dolphin is launched, which is inconsistent.

> > 3. relaunch of panorama wizard within a given digikam session does not re-check
> > existance of hugin apps (only after digikam is restarted and the panorama
> > creator reopened does it detect moved or missing apps), so if the hugin is
> > installed/uninstalled/moved while digikam is opened then the panorama app may
> > not notice the change.
> 
> Good point (I didn't thought of that because I'm using the standalone version
> of the plugin for testing). It should be working now.

Unfortunately this is not working :(

> > 4. an 'Error - Preview cannot be processed.' occurs at the 'Preview and
> > Post-Processing' page. This did not happen with code I pulled on 2012-01-11.
> > Could be related to problem 1?
> 
> Is this error still happening with the latest commits?

It was still happening, but I fixed it. The problem was that make could not find nona. This was because in the .mk file nona app was listed as 'NONA="nona"' and not 'NONA=nona', so the string replace failed. If you still need the former method on unix then maybe a regex can be used, but be sure to leave the double-quotes or the windows path won't work properly because it is highly likely to have a space in it somewhere!

So, there are two problems left.

1. After locating one of the executables, all the remaining executables are not located automatically in the same path. Instead, each has to be located individually. This worked before your changes yesterday.

2. If an error occurs at some point in processing, the 'Next' button is still available.

3. Same problem as 3. above

Almost there!

(In reply to comment #44)
> Doesn't crash anymore (pto2mk.exe was crashing)

How could you tell that? Was it because you ran it on your own and found out it was crashing? If not, the user should not be aware of a crash of pto2mk, only that it is not detected, or that the version is detected as not recent enough...

> > > 3. relaunch of panorama wizard within a given digikam session does not re-check
> > > existance of hugin apps (only after digikam is restarted and the panorama
> > > creator reopened does it detect moved or missing apps), so if the hugin is
> > > installed/uninstalled/moved while digikam is opened then the panorama app may
> > > not notice the change.
> > 
> > Good point (I didn't thought of that because I'm using the standalone version
> > of the plugin for testing). It should be working now.
> 
> Unfortunately this is not working :(

Then, I have to setup a test environment for that.

> 1. After locating one of the executables, all the remaining executables are not
> located automatically in the same path. Instead, each has to be located
> individually. This worked before your changes yesterday.

Done (I introduced a bug during one of my commits from yesterday).

> 2. If an error occurs at some point in processing, the 'Next' button is still
> available.

Damn it! I fixed the 'Back' button, and now the 'Next' one doesn't work :'( Can you tell me at which point of the process you've seen this behavior? I don't think the bug is present at any point of the process (for example, it is working as expected in the intro page...)

(In reply to comment #45)
> (In reply to comment #44)
> > Doesn't crash anymore (pto2mk.exe was crashing)
> 
> How could you tell that? Was it because you ran it on your own and found out it
> was crashing? If not, the user should not be aware of a crash of pto2mk, only
> that it is not detected, or that the version is detected as not recent
> enough...

On Windows, if a program crashes a window is diplayed informing the user of this and asking what to do, such as submitting a bug report to Microsoft, restarting the program, or (if installed) launching debug tools. When the scan happened, one of these windows would pop up telling me of that pto2mk.exe crashed.

> > > > 3. relaunch of panorama wizard within a given digikam session does not re-check
> > > > existence of hugin apps (only after digikam is restarted and the panorama
> > > > creator reopened does it detect moved or missing apps), so if the hugin is
> > > > installed/uninstalled/moved while digikam is opened then the panorama app may
> > > > not notice the change.
> > > 
> > > Good point (I didn't thought of that because I'm using the standalone version
> > > of the plugin for testing). It should be working now.
> > 
> > Unfortunately this is not working :(
> 
> Then, I have to setup a test environment for that.

Perhaps destroying the panorama wizard after it is closed/finished would be sufficient to re-initiate the check?

> > 1. After locating one of the executables, all the remaining executables are not
> > located automatically in the same path. Instead, each has to be located
> > individually. This worked before your changes yesterday.
> 
> Done (I introduced a bug during one of my commits from yesterday).

Works great now, thanks.

> > 2. If an error occurs at some point in processing, the 'Next' button is still
> > available.
> 
> Damn it! I fixed the 'Back' button, and now the 'Next' one doesn't work :'( Can
> you tell me at which point of the process you've seen this behavior? I don't
> think the bug is present at any point of the process (for example, it is
> working as expected in the intro page...)

I've seen it happen several times. I can reproduce it on the 'Pre-Processing Images' page after 'Pre-processing has failed' for instance if I move the location of hugin without updating the panorama wizard, and also on the 'Preview and Post-Processing' page after the make file could not be processed (error shown is 'Preview cannot be processed') which occurs if the nona is not in the location the makefile shows.

I also noticed a new problem, so in total there are 3 minor problems:

1. On the final page of the wizard the user is asked to choose a 'file name template'. If a name is chosen which is the same as an existing image then clicking 'Finish' does nothing and no warning/error is presented to the user. Only after changing the name to something new does 'Finish' do anything. So if a warning could be presented to the user, that would be great.

2. Same error as 2 before, next button still functional after error

3. Same error as 3 before, relaunch of panorama wizard does not check if executables are still in the same locations

Git commit 8a623d8a55c740bbaee4a1b179f8a53348297ff6 by Benjamin Girault.
Committed on 23/01/2012 at 20:17.
Pushed by girault into branch 'master'.

Panorama: Bugfix for binary detection recheck

In the case of a single digikam session.

M  +5    -0    common/libkipiplugins/tools/binaryiface.cpp
M  +1    -7    common/libkipiplugins/widgets/binarysearch.cpp
M  +11   -0    panorama/manager/manager.cpp
M  +2    -0    panorama/manager/manager.h
M  +0    -2    panorama/plugin/panorama.cpp
M  +1    -2    panorama/plugin/plugin_panorama.cpp

http://commits.kde.org/kipi-plugins/8a623d8a55c740bbaee4a1b179f8a53348297ff6

(In reply to comment #46)
> On Windows, if a program crashes a window is diplayed informing the user of
> this and asking what to do, such as submitting a bug report to Microsoft,
> restarting the program, or (if installed) launching debug tools. When the scan
> happened, one of these windows would pop up telling me of that pto2mk.exe
> crashed.

Hum. I guess (or rather I hope) that there might be some way to tell windows that a crash of a child program is handled internally... but I do not have the knowledge to investigate that.

> [relaunch bug inside a single digikam session]
> 
> Perhaps destroying the panorama wizard after it is closed/finished would be
> sufficient to re-initiate the check?

Can you try again with the new patch? It should be working now.

> I've seen it happen several times. I can reproduce it on the 'Pre-Processing
> Images' page after 'Pre-processing has failed' for instance if I move the
> location of hugin without updating the panorama wizard, and also on the
> 'Preview and Post-Processing' page after the make file could not be processed
> (error shown is 'Preview cannot be processed') which occurs if the nona is not
> in the location the makefile shows.

At the moment, it is more like a feature than a bug: I meant for that. The reason is that the plugin do not bother with the result of the preview compilation and allow the user to continue without the preview. I guess that I can make something smarter, like if there is a real error (not a cancellation of the preview compilation) then the Next button is hidden. However, the plugin is then blocked and can be closed (I do not see any reasonable condition to re-enable the Next button).

> On the final page of the wizard the user is asked to choose a 'file name
> template'. If a name is chosen which is the same as an existing image then
> clicking 'Finish' does nothing and no warning/error is presented to the user.
> Only after changing the name to something new does 'Finish' do anything. So if
> a warning could be presented to the user, that would be great.

Yep, I know that, and it is on my TODO list ;o) I'll upgrade it to my TODO ASAP list ;o)

(In reply to comment #48)
> (In reply to comment #46)
> > I've seen it happen several times. I can reproduce it on the 'Pre-Processing
> > Images' page after 'Pre-processing has failed' for instance if I move the
> > location of hugin without updating the panorama wizard, and also on the
> > 'Preview and Post-Processing' page after the make file could not be processed
> > (error shown is 'Preview cannot be processed') which occurs if the nona is not
> > in the location the makefile shows.
> 
> At the moment, it is more like a feature than a bug: I meant for that. The
> reason is that the plugin do not bother with the result of the preview
> compilation and allow the user to continue without the preview. I guess that I
> can make something smarter, like if there is a real error (not a cancellation
> of the preview compilation) then the Next button is hidden. However, the plugin
> is then blocked and can be closed (I do not see any reasonable condition to
> re-enable the Next button).

Sorry, my comment applies only to the Preview Page.

For the Preprocessing Page I did not understand what you did. I removed one of the binaries after its detection by the plugin and proceeded. The Next button has not been re-enabled after the error 'Pre-processing has failed'. Same for the Optimisation Page.

(In reply to comment #48)
> (In reply to comment #46)
> > On the final page of the wizard the user is asked to choose a 'file name
> > template'. If a name is chosen which is the same as an existing image then
> > clicking 'Finish' does nothing and no warning/error is presented to the user.
> > Only after changing the name to something new does 'Finish' do anything. So if
> > a warning could be presented to the user, that would be great.
> 
> Yep, I know that, and it is on my TODO list ;o) I'll upgrade it to my TODO ASAP
> list ;o)

Done.

Ananta: have you got the time to test the latest commits?

(In reply to comment #51)
> Ananta: have you got the time to test the latest commits?

Yes, sorry, I have been a bit busy. Only problem 2 and 3 from comment #46 are still present, but I don't think these should prevent you from closing this bug. It seems unlikely that the user will move an executable while digikam is running, and having 'next' enabled when it shouldn't be doesn't actually cause any harm (just doesn't behave as one would expect). Perhaps a new bug could be opened with just these two issues and they could be made windows specific? I tried the stand-alone version of the app and the 'next' button being active on error behaves exactly the same as within digikam.

Git commit 94b33e8b7f386771c17016623ee746e31a98dd74 by Benjamin Girault.
Committed on 18/03/2012 at 19:37.
Pushed by girault into branch 'master'.

Panorama: Next button disabled after error

 * upon receiving an error during preview generation
(* also string localisation added in previewpage)

M  +11   -0    panorama/importwizard/importwizarddlg.cpp
M  +1    -0    panorama/importwizard/importwizarddlg.h
M  +12   -8    panorama/importwizard/previewpage.cpp
M  +1    -0    panorama/importwizard/previewpage.h

http://commits.kde.org/kipi-plugins/94b33e8b7f386771c17016623ee746e31a98dd74

After digging a bit, I think I finally understood correctly what was the faulty behavior when an error occurs during preview generation. My latest commit should have corrected that.

About the issue of not rechecking binaries within a single digikam session, can you open another bug if it is still the case? I looked at the code, and do not see why it is not working.

I'm closing the bug report since the initial bug is fixed.