simple-scan-team team mailing list archive

[Robert Ancell <robert.ancell@xxxxxxxxxxxxx>]

Review: Needs Fixing

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).
-- 
https://code.launchpad.net/~gotwig/simple-scan/headerbars/+merge/220878
Your team Simple Scan Development Team is subscribed to branch lp:simple-scan.