simple-scan-team team mailing list archive

[Robert Ancell <robert.ancell@xxxxxxxxxxxxx>]

Review: Needs Fixing

What I like here is the removal of the menubar and the cog menu design - that is good to go upstream.

Issues with the branch as it stands:
- You have a hard dependency on granite - this needs to be optional.
- In saying that, I don't see the value in us taking a dependency on granite since it's Elementary specific. So I'd expect to get the majority of the functional changes into upstream simple-scan but still need a small patch for Elementary specific functionality.
- By removing the menu you have broken the right click functionality on a page.
- The cog menu is missing important functionality like email and print.
- You don't need the crop option in the cog menu (it is in right click on the page). Though it might be worth having the whole page menu as a submenu so it is discoverable.
- The about dialog renames and points at lp:elementary-scan - if this is upstreamed code it should point at lp:simple-scan
- Coding style doesn't match existing code (whitespace, comment style etc).
- Unnecessary changes in the branch - newline at the end of a function.

What I recommend you do is make this branch:
- Remove the current menubar (keeping the right click menu)
- Add the cog menu and populate with existing functionality.

We can then look in a second patch the value of adding an optional granite dependency.
-- 
https://code.launchpad.net/~teemperor/simple-scan/elementary-ui/+merge/147199
Your team Simple Scan Development Team is subscribed to branch lp:simple-scan.