jcelaya / hdrmerge

HDR exposure merging
http://jcelaya.github.io/hdrmerge/
Other
355 stars 78 forks source link

Show MainWindow on launch when no files passed #167

Closed ff2000 closed 5 years ago

ff2000 commented 5 years ago

HdrMerge directly calls MainWindow::loadImages in showEvent which opens a dialog that blocks the current EventLoop preventing the MainWindow from showing up. That is confusing (to me) as only a file dialog pops up. The quick solution is to postpone the loadImages call so that the MainWindow shows up:

diff --git a/src/MainWindow.cpp b/src/MainWindow.cpp
index 3e97b8e..1131b43 100644
--- a/src/MainWindow.cpp
+++ b/src/MainWindow.cpp
@@ -241,7 +241,7 @@ void MainWindow::about() {

 void MainWindow::showEvent(QShowEvent * event) {
     if (io.getImageStack().size() == 0) {
-        loadImages();
+        QMetaObject::invokeMethod(this, "loadImages", Qt::QueuedConnection);
     }
 }

A better approach IMO would be to just open the MainWindow and present the user a "Open Raw images" button. Could also be used as a "Welcome Screen" with different other actions (show help, Go to homepage, last edited images, etc.)

Beep6581 commented 5 years ago

Showing the loadImages window seems logical to me. Is there anything you can do in the main window when no images are loaded?

ff2000 commented 5 years ago

As I said: add a button to manually open the Load Files dialog. Of course that will make more sense when there are several other buttons (Show Help etc.) This was the first time I started hdrmerge so I do not yet know what it is capable of and which actions the user could trigger. Another option would be to show the LoadOptionsDialog as centralWidget in the MainWindow. Or show the empty editor with an overlay widget that tells the user that no images are loaded and present a "load images" button.

In any case the current situation IMO needs to be improved. I was confused when I didn't see a proper Window on startup, just file picker. My first thought: This will just dump the finished HDR to disk. I selected a file and got the next dialog.

Now play confused user: Start hdrmerge and close the dialog. MainWindow shows up. Minimize it and maximize it again - and the fun can start ;) (simply open some raw files and press "Accept" to merge)

ff2000 commented 5 years ago

As the LoadOptionsDialog also uses the showEvent you can play with that, too. Start hdrmerge, select files with the file picker, then minimize the LoadOptionsDialog and maximize it again. Those issues should be solved in any case, a different startup and load procedure is IMO the best and easiest way.

ff2000 commented 5 years ago

I just found another issue: MainWindow::io is queried in showEvent but to that point not yet processed - that takes place in loadImages. I am unsure if there should be a loadoptionsdialog at all if the user passes files as program arguments. There are args for align etc. so there is no reason to pop up that dialog. If we can agree on a way how this startup+loading should happen I can do the implementation. I am fine with showing LoadOptionsDialog but would probably not automatically pop up a filemanager so the user has to click on "add". Any objections?

Beep6581 commented 5 years ago

Now play confused user: Start hdrmerge and close the dialog. MainWindow shows up. Minimize it and maximize it again - and the fun can start ;) (simply open some raw files and press "Accept" to merge)

Yes, odd things happened. For reference, after the main window was minimized, showing it popped up the "Open raw images" dialog again, and clicking "Cancel" made the dialog pop up again. I had to "Cancel" it three times for it to stop popping up.

I am unsure if there should be a loadoptionsdialog at all if the user passes files as program arguments.

The dialog should still pop up so that the user can specify those options when launching HDRMerge by selecting several files in a file browser and invoking Open With > HDRMerge.

ff2000 commented 5 years ago

I am unsure if there should be a loadoptionsdialog at all if the user passes files as program arguments.

The dialog should still pop up so that the user can specify those options when launching HDRMerge by selecting several files in a file browser and invoking Open With > HDRMerge.

Ah, OK, thx!

Could you try this patch if it works for you?

diff --git a/src/Launcher.cpp b/src/Launcher.cpp
index b188d79..a01b419 100644
--- a/src/Launcher.cpp
+++ b/src/Launcher.cpp
@@ -51,6 +51,7 @@ int Launcher::startGUI() {
     MainWindow mw;
     mw.preload(generalOptions.fileNames);
     mw.show();
+    QMetaObject::invokeMethod(&mw, "loadImages", Qt::QueuedConnection);

     return QApplication::exec();
 #else
diff --git a/src/LoadOptionsDialog.cpp b/src/LoadOptionsDialog.cpp
index f846929..3d5fbea 100644
--- a/src/LoadOptionsDialog.cpp
+++ b/src/LoadOptionsDialog.cpp
@@ -104,17 +104,12 @@ LoadOptionsDialog::LoadOptionsDialog(QWidget * parent, Qt::WindowFlags f)

 void LoadOptionsDialog::showEvent(QShowEvent * event) {
-    if (fileNames.size() == 0) {
-        addFiles();
-    } else {
+    if (!fileNames.empty()) {
         for (auto & i : fileNames) {
             new FileItem(i, fileList);
         }
         fileNames.clear();
     }
-    if (fileList->count() == 0) {
-        QMetaObject::invokeMethod(this, "reject", Qt::QueuedConnection);
-    }
 }

diff --git a/src/MainWindow.cpp b/src/MainWindow.cpp
index 3e97b8e..a0d327d 100644
--- a/src/MainWindow.cpp
+++ b/src/MainWindow.cpp
@@ -238,14 +238,6 @@ void MainWindow::about() {
     dialog.exec();
 }

-
-void MainWindow::showEvent(QShowEvent * event) {
-    if (io.getImageStack().size() == 0) {
-        loadImages();
-    }
-}
-
-
 void MainWindow::loadImages() {
     LoadOptionsDialog lod(this);
     if (!preloadFiles.empty()) {
diff --git a/src/MainWindow.hpp b/src/MainWindow.hpp
index 648a247..2329f4e 100644
--- a/src/MainWindow.hpp
+++ b/src/MainWindow.hpp
@@ -46,7 +46,6 @@ class MainWindow : public QMainWindow {
 public:
     MainWindow();

-    void showEvent(QShowEvent * event);
     void closeEvent(QCloseEvent * event);
     void preload(const std::vector<QString> & o) {
         preloadFiles = o;
Beep6581 commented 5 years ago

I will try to test it tomorrow, please poke me if I forget.

ff2000 commented 5 years ago

One small issue: The LoadOptionsDialog and the File Selector in LoadOptionsDialog::addFiles both have the same window title. The File Selector then will have a "<2>" appended to the title. IMO one should be renamed - e.g. "Select RAW files" for the file selector to keep the Menu action and the title of the LoadOptionsDialog the same. Not sure about "files" vs. "images".

Beep6581 commented 5 years ago

"Select Raw Photos" for the window title.

ff2000 commented 5 years ago

I pushed the changes to my fork: https://github.com/ff2000/hdrmerge/commit/60b1df5f6c8eb5c4fe6871101d9e973ee0f1d23b

ff2000 commented 5 years ago

Any opinion yet on the changes? Should I create a pull request?

Beep6581 commented 5 years ago

I haven't had time to test yet and probably won't be able to this weekend. Go ahead with the PR, at least it puts this in a visible place. I will test as soon as I can.

ff2000 commented 5 years ago

OK, I will create a PR ASAP.

Beep6581 commented 5 years ago

Tested and fix confirmed.