Merge lp://qastaging/~gotwig/simple-scan/headerbars into lp://qastaging/~simple-scan-team/simple-scan/trunk

The bottom line is missing. I have no idea how to add it... Please help if you can.

The new layout looks nice! Thanks.

- You can't access the preferences unless the scan fails - you need to be able to access these before starting a scan.
- The rotate buttons seem the wrong way around to me; did you switch them for a particular reason?
- There are some document level options that are no longer accessible, i.e. page reordering, email. We should be able to access these somehow - a drop down menu of less frequent options?
- You can't stop the scan - I agree that stop shouldn't be a top-level option but there should be a visible way to cancel a scan while in progress (a button over the currently scanning page?). This is because scans can take a long time to complete.
- The bottom of the pages seems to be clipped. Is this what you mean by the bottom line comment?
- We need to support non-headerbar operation for Ubuntu so both methods need to work.
- For some reason running a test scan fails (run "simple-scan test" to use the test backend). I can't immediately see how your changes could affect it though.

Thanks for the feedback.

Could you take a look at the "bottom line issue"? I really have no idea how it works :)

I am trying to work on the other issues as good as I can.

@gotwig: you can avoid a large amount of the diff if you drop the additiontal indentation of 4 spaces in the whole file

Some answers to your critics

- You can't access the preferences unless the scan fails - you need to be able to access these before starting a scan. Answer: I didt change this behaviour. It was like it was before.
- The rotate buttons seem the wrong way around to me; did you switch them for a particular reason?
Answer: you are right. I will do them the right way, and add them into a link box.
- There are some document level options that are no longer accessible, i.e. page reordering, email. We should be able to access these somehow - a drop down menu of less frequent options?
Answer: I dont understand. I changed nothing about this??

- You can't stop the scan - I agree that stop shouldn't be a top-level option but there should be a visible way to cancel a scan while in progress (a button over the currently scanning page?). This is because scans can take a long time to complete.
Answer: I will be working on this. I try to fix this with a normaly invisible button for STOP, becoming visible, when a scan is in progress, replacing the "scan" button.

- The bottom of the pages seems to be clipped. Is this what you mean by the bottom line comment?
Answer: I hope you can work on that. I tried long and got no result. But when I tried the testscan, which btw should be documented in the simplescan manpage pls!, the bottom line for new pages was indeed there.
- We need to support non-headerbar operation for Ubuntu so both methods need to work.
Answer: I try to work on this. So we get the classical Toolbar back for Ubuntu only?
- For some reason running a test scan fails (run "simple-scan test" to use the test backend). I can't immediately see how your changes could affect it though.
Answer: The test scan worked for me.

- There are some document level options that are no longer accessible, i.e. page reordering, email. We should be able to access these somehow - a drop down menu of less frequent options?
Answer: I get what you mean. I try to work on a gnome headerbar solution for this.
Probably I will create a dropdown, and put all document-related actions in there, to save space.

So I will add rotate and cut there too.

I would be happy if you could take a look at it again.

Thanks Eduard! Patch is improving.

I was using the new Launchpad support for inline comments, I'm guessing they didn't show for you.

Things still needing fixing:
- Duplicating the whole .ui file for showing the HeaderBar is too hard to maintain. The .ui should contain a HeaderBar, menu and toolbar. For Systems that support it the HeaderBar should be shown and the menu and toolbar hidden. The opposite for systems that do not support HeaderBars.
- While you have added back some missing buttons to access document level functions there is still not a way to access preferences. Also, it looks very cluttered - did you investigate putting the less used options into a menu?
- You changed the return value of button_cb in book-view.vala to return true instead of false. Was there a reason for this change?
- You have some tab characters where they should be spaces
- You've changed the default window size but this doesn't seem necessary for this patch - does it not look correct with the existing defaults?

As Rico said, please investigate if you can make the .ui files easier to diff. It is very hard to review the changes as they are currently (a big downside of using .ui files :( ).

I think the bottom being clipped off the page is due to the border size changes. I will have a look later when I have time (I am currently travelling).

I tried to not duplicate the .ui file, but (!), there is no way to enable/disable headerbars trough .ui logic. I tried it. What you have to do, is to remove the "type="titlebar" option from the <child> tag. You cant do this. I found no way to do this.

There is a way to access the preferences. There is an entry in the AppMenu for this. This is following GNOME HIG.

There was a reason for this change, because if I wouldnt have done this, there would be no right-click in the headerbar version. Try it yourself.

I have no idea how to do the spaces/tabs/indentation. Im sorry.

The window size thing should be reverted, I agree. But this is very easy todo. I experimented with this, because I wanted to enable the most minimium size by default for the window.

Thanks for your review. I think about howto put the less used actions into some submenu.

I think that if you want to add the header bar only conditionally, then you should not attempt to do so in the UI file at all; this is doomed. Create the header bar entirely in Vala, pack your buttons into it, then grab your GtkApplicationWindow out of the UI file using GtkBuilder in order to set it as the titlebar. It's much, much nicer to define the header bar in a UI file than in Vala, but only if you want to use it unconditionally. Mahjongg is probably the best example of how to do this in Vala: https://git.gnome.org/browse/gnome-mahjongg/tree/src/gnome-mahjongg.vala

P.S. To convert tabs to spaces, open the file in gedit, press Ctrl+H to do a search and replace, type \t in the search box and the desired number of spaces in the replace box.

P.S.S. Also, saving the toolbar toolbuttons and header bar toolbar buttons separately is not a good idea; note that Gtk.ToolButton is derived from Gtk.Button so you should not need two different variables.

I've pushed a branch to lp:~robert-ancell/simple-scan/optional-headerbar-ui that shows how you can have both the header bar and the toolbar in the same .ui file and conditionally use each one.

You disable the HeaderBar by using Gtk.Window.set_titlebar (null). This doesn't work in GTK+ 3.10 because it doesn't have [1] but I will backport that to Ubuntu.

I'm also open to changes that do the UI in the .vala files. Vala is not too bad at being concise in generating UI and it makes the patches easier to read.

[1] https://git.gnome.org/browse/gtk+/commit/?h=gtk-3-12&id=8db2ba425aaff107faaa4c7468be63f8ef2fa8e9

Regarding the size changes - these aren't the minimum sizes but the default size. The numbers you have chosen are probably related to the particular theme you are using and will probably change over time as the theme is changed.

I recommend removing these changes from this branch and they can be re-proposed as a later merge proposal. Smaller branches are easier to review.

Hey Robert,

I would welcome it, if you could take a look at this bug report as well.
https://bugs.launchpad.net/simple-scan/+bug/1325760

Its somewhat related to this headerbar change as well.

We could move the buttons from the headerbar, to a GtkActionBar. And its not so hard to do, I just dont know yet where to place it. Would be nice if you could take a look, and tell me where the right place would be to add a GtkActionBar as a child of a page for the book view.

Good news: I finally fixed the border problem.

I can reproduce the problem with no image after a scan.

The part about the broken image scan functionality is a problem of simple scan trunk, master branch and not of this branch.

Please fix the whitespace changes in the .ui file - I've made p:~robert-ancell/simple-scan/headerbars-ui-whitespace with these fixed, please merge/copy in those changes.

Please drop the changes to the man page - they don't relate to this merge.

Note that 'simple-scan test' works because SANE has a 'test' driver compiled in by default. It's probably worth mentioning that in the man page, and not that it's not guaranteed to be provided. This should be a separate merge though.

Looking at the diff below you seem to have moved the app menu logic inside the try clause for no reason - please move it back to the original location. Also there is a whitespace change on line 2521 which doesn't seem relevant.

With those changes this diff should be the appropriate size for this enhancement.

A number of buttons don't work after these changes - it's because of the object renames you did in the .ui file - check references to these are updated both in the .ui file and in the .vala files.

I will update the button names. "Looking at the diff below you seem to have moved the app menu logic inside the try clause for no reason - please move it back to the original location. Also there is a whitespace change on line 2521 which doesn't seem relevant."

Ok, I did the app menu logic in another line, to not check twice for has_app_menu().

In your version, the part was missing where you actually add the appmenu.ui file if the application has an app_menu.

Please check it again...

In your merge you replaced all my GtkButtons with GtkToolButtons again.
This is against the GNOME HIG, I have to disapprove this merge.

Thanks for fixing the .ui file indentation - it's much easier to read now.

You haven't done the replaces in src/ui.vala - for example on line 1670 there is a reference to "text_toolbutton_menuitem" but you've renamed it to "text_menuitem".

Please move the app menu logic back - there's no problem calling has_app_menu () more than once and it makes the try clause contain more code than necessary. We are only checking for UI load failures at this point.

btw You should change your review to "Approve" unless you disapprove of your own code!

I don't know if this is really the way to do buttons for the headerbar.
My version with a seperate UI files used GtkButtons instead of GtkToolButtons, and also, when I tested with Ubuntu 14.04, it worked. When I now test your version with the headerbar version, I see GtkToolButtons instead of GtkButtons, they look different. And also your version fails to work for me under Ubuntu 14.04. I dont see window rendering, only the content. I tried to remove every toolbutton from the new version.. thats the reason we have now these references without toolbutton ...

When I talked to users in GTK+ in IRC on GIMPNet, they actually told me that soon toolbars are going to be deprecated. I fear when this happens, we cant use GtkToolButtons anymore as well.

Eduard, you shouldn't need to modify any of the existing code. Your additions don't add GtkToolButtons, and the existing code continues to work as it did before.

I talk about wanting to use GtkButtons not GtkToolButtons

This merge request is "Add headerbars" - the current changes do that and don't add any use of GtkButton. The best way to make changes is to keep them as small as possible. If there's an issue with the existing use of GtkToolButton then this should be proposed in a separate merge.

Ok, I will sort out your issues soon and approve it.

I renamed the buttonnames, and added the old load appmenu logic back.

Eduard - my apologies I misread your comments regarding the less frequently used options. As you correctly stated they are in the appmenu (which wasn't showing in Unity when I was testing it). So I have removed those options and let the user use the appmenu instead.

I added the reorder pages to the appmenu as it was never added there.

Merged with fixed to the indentation added in your last commit.

Thanks for your work!

Also regarding the use of Launchpad - when you set the review field to "resubmit" you are asking yourself to resubmit the proposal (kind of sounds like you have a split personality). In Launchpad you should never need to review your own code - it's implied you approve of your own proposal!! Not a problem, just looks kind of funny :) It's a Launchpad UI bug I guess - a proposer should probably not see the review options since they're not applicable.

Resubmitting is a method of proposing where you can generate a new merge request for this proposal (obsoleting the current one). I don't know of any projects that use that since just making new commits and then merging when ready seems sufficient for me.

I opened bug 1330333 about the Launchpad UI issue.