Is there a way to block interaction with Glom while the InfoBar is displayed? The dialog created with Utils::get_and_show_pulse_dialog() was made modal, but how should the InfoBar achieve the same behaviour?

I guess you would need to add something to do that. Maybe calling set_sensitive(false) on the entire main window (see get_application()) if that doesn't look too awful?

Created attachment 198513 [details] [review]
incomplete patch to replace dialog with infobar

This incomplete patch replaces the dialog with an infobar. Unfortunately, it does not show the infobar. I also have not attempted to change the sensitivity of the main window yet.

(In reply to comment #3)
> Created an attachment (id=198513) [details] [review]
> incomplete patch to replace dialog with infobar
> 
> This incomplete patch replaces the dialog with an infobar. Unfortunately, it
> does not show the infobar.

Probably because it tries to add itself to a single-item container (GtkWindow) that already has a child, and it doesn't show() itself:

+ Application* app = dynamic_cast<Application*>(Application::get_application());
+  if(app)
+    app->add(*this); 

Instead of trying to use the existing strategy for showing windows, I think it would be wiser to add a GtkInfoBar in the main window's glade file, and just hide/show it when appropriate, changing its contents as appopriate. It if's always used for progress, then a derived Gtk::InfoBar would make sense to provide a simple API for that.

Created attachment 198848 [details] [review]
updated patch based on comment #4

Attaching an updated patch. Although the infobar is present in the main window glade file, I guess that I have to add it to the frame's vbox in order for it to be shown?

(In reply to comment #5)
> Although the infobar is present in the main window
> glade file, I guess that I have to add it to the frame's vbox in order for it
> to be shown?

No, you should be able to leave it where it is.

And maybe set_progress_message() should be a method of Application rather than Frame. After all, I guess that the InfoBar is a more-direct child of Application rather than a child of Frame.

The get_application() method might help.

Created attachment 198928 [details] [review]
updated patch based on comment #7

The patch is mostly working now, except for the label packed into the infobar content area not showing markup, and the infobar not being hidden during startup. I am working on adding some logic to clear the progress message once an operation completes, so this should be working soon.

Great. It's good to see so much code deleted.

Created attachment 199007 [details] [review]
functional patch, except label markup

I updated the patch again, to solve the problem where the message was not hidden when set from methods that could exit from multiple return points. I added a class inside Glom::Application that clears the message inside the destructor, which solves the problem but seems ugly. A full solution could use a stack approach like GtkStatusbar, but I am not sure if the added complexity is worth the benefits.

Review of attachment 199007 [details] [review]:

::: glom/application.cc
@@ +1638,3 @@
 bool Application::recreate_database_from_example(bool& user_cancelled)
 {
+  ClearProgressMessage clear;

It's a bit odd that this isn't used in other places (in other classes), where Application::set_progress_message() is called.

I guess it would make more sense to instantiate a
  ShowProgressMessage("My message")
everywhere, that would take care of everything.

I generally like the use of the destructor to make sure that it's cleared later. We can add a static stack inside that class if that become necessary, but hopefully we don't need it now.

::: glom/infobar_progress_creating.cc
@@ +29,3 @@
+
+const char* Infobar_ProgressCreating::glade_id("infobar_progress");
+const bool Infobar_ProgressCreating::glade_developer(false);

These (and the instantiation test, which is the only thing that uses them) shouldn't be necessary any more. Instead it is instantiated by a get_widget_derived() call in the main windows' constructor, where we specify the ID.

Also, it currently shows markup, instead of using markup. And the widgets are arranged rather arbitrarily at the right. Maybe a horizontal arrangement would make more sense.

Created attachment 199621 [details] [review]
use new ShowProgressMessage class

Updated patch attached, which renames ClearProgressMessage to ShowProgressMessage and uses it in Application and Frame_Glom.

So is this one ready?

(In reply to comment #15)

No, it segfaults:

rogram received signal SIGSEGV, Segmentation fault.
memcpy () at ../sysdeps/x86_64/memcpy.S:423
423	../sysdeps/x86_64/memcpy.S: No such file or directory.
	in ../sysdeps/x86_64/memcpy.S
(gdb) bt full

+ Trace 228880

Also, I have not yet been able to fix the markup problem or the alignment of the content inside the InfoBar.

valgrind should tell you what's going wrong. It looks like a null pointer.

Yes the SlotProgress that is called by Glom::Spawn::execute_command_line_and_wait has no arguments, but I added ShowProgressMessage as an argument to the slots in Application. I do not know libsigc++ well, but is it posible to resolve this while still passing around a SlotProgress inside libglom? Alternatively, would it be easier to just add the ShowProgressMessage argument to SlotProgress?

Review of attachment 199621 [details] [review]:

Yes, I'd like to avoid that use of sigc::bind() if possible. Maybe you can use my suggestion here and then see what is left.

::: glom/application.cc
@@ +1271,2 @@
 {
+  progress_message.pulse();

Passing that whole Object (and by value via bind() which has its own complications) just to do a pulse seems overcomplicated. At least in this case, isn't it enough to just call some Application::pulse_status() (or suchlike) method?

(In reply to comment #19)
> Passing that whole Object (and by value via bind() which has its own
> complications) just to do a pulse seems overcomplicated. At least in this case,
> isn't it enough to just call some Application::pulse_status() (or suchlike)
> method?

Well, in the case that pulse() could be moved into Application, there would be two different ways to control the progress: the ShowStatusMessage and calling Application methods directly. To be honest, I am wholly unhappy with the current implementation, but I do not feel confident enough with Glom or libsigc++ to be able to suggest any better one.

It seems OK that ShowStatusMessage is just a sometimes-convenient way to use the Application's methods. Could you try making it simple where that's useful, and then see how it looks?

Created attachment 199637 [details] [review]
no segfaults, still have to fix markup and layout

Does not crash now, but stil have the markup and layout problems.

Created attachment 199644 [details] [review]
working, except for incorrect markup display

Created attachment 199652 [details] [review]
working patch

Now with working markup!

Thanks. Pushed with some whitespace/code-style changes and removal of unrelated whitespace/code-style changes.

I have also made a few further commits. In particular, dealing with the TODO to block user input.