Merge lp://qastaging/~teemperor/simple-scan/elementary-ui into lp://qastaging/~simple-scan-team/simple-scan/trunk

Proposed by Raphael Isemann
Status: Rejected
Rejected by: Robert Ancell
Proposed branch: lp://qastaging/~teemperor/simple-scan/elementary-ui
Merge into: lp://qastaging/~simple-scan-team/simple-scan/trunk
Diff against target: 148 lines (+74/-5)
3 files modified
configure.ac (+1/-0)
src/Makefile.am (+2/-1)
src/ui.vala (+71/-4)
To merge this branch: bzr merge lp://qastaging/~teemperor/simple-scan/elementary-ui
Reviewer Review Type Date Requested Status
Robert Ancell Needs Fixing
Review via email: mp+147199@code.qastaging.launchpad.net

Description of the change

I changed the UI as seen here ( https://lists.launchpad.net/elementary-dev-community/msg01568.html ) to meet the elementary-HIG.

My code shouldn't affect simple-scan on other desktops then pantheon in any way.

I get the DE-name from the DESKTOP_SESSION variable, so to test the changes from another distribution you need to run "export DESKTOP_SESSION=pantheon" before executing simple-scan (from the same shell as the export-command).

New dependencies are only the granite library.

For quick feedback i'm also available on #elementary-dev (freenode) as "teemperor".

Have a nice evening!

To post a comment you must log in.
Revision history for this message
Robert Ancell (robert-ancell) wrote :

So sorry! It appears I wasn't subscribed to merge requests so I didn't see this merge (I was expecting it though). Feel free to ping me directly if you think I've missed something like this.

Revision history for this message
Robert Ancell (robert-ancell) wrote :

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.

review: Needs Fixing
Revision history for this message
Eduard Gotwig (gotwig) wrote :

this code is outdated, and the merge request should be removed.

Unmerged revisions

606. By Raphael Isemann

removed libgee from dependencies

605. By Raphael Isemann

removed the contractor-section that was already commented out

604. By Raphael Isemann

added an optional ui that meets the elementary-guidelines

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches