Merge lp://qastaging/~victor-mireyev/simple-scan/484616 into lp://qastaging/~simple-scan-team/simple-scan/trunk

Proposed by Victor Mireyev
Status: Rejected
Rejected by: Robert Ancell
Proposed branch: lp://qastaging/~victor-mireyev/simple-scan/484616
Merge into: lp://qastaging/~simple-scan-team/simple-scan/trunk
Diff against target: 98 lines (+28/-10)
3 files modified
src/book.vala (+21/-6)
src/jpeglib.vapi (+5/-0)
src/page.vala (+2/-4)
To merge this branch: bzr merge lp://qastaging/~victor-mireyev/simple-scan/484616
Reviewer Review Type Date Requested Status
Robert Ancell Disapprove
Review via email: mp+167359@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2013-05-29.

Description of the change

Save DPI with JPEG

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

Can you remove the JpegWriter class back to how it was? It doesn't seem to add anything and it makes it harder to review.

Please correct the whitespace before the '(' in the use of functions.

Other than that, looks good.

review: Needs Fixing
Revision history for this message
Robert Ancell (robert-ancell) wrote : Posted in a previous version of this proposal

Is this an enum? Can you put the enum values into the .vapi if so?
+ info.density_unit = 1;

Revision history for this message
Victor Mireyev (greymouse2005) wrote : Posted in a previous version of this proposal

As you can see, there are three important changes:
1) optional density parameter is added
2) n_written parameter is removed as it's trivially obtained via jpeg_data.length
3) Quality and density code is added
92+ info.set_quality (90);
93 + if (density != null)
94 + {
95 + info.density_unit = 1;
96 + info.X_density = (uint16) density;
97 + info.Y_density = (uint16) density;
98 + }

There are two reason for existance of JpegWriter class.

1) JPEG compression code doesn't use any nonstatic methods or properties of the Book class.
2) As JPEG compression code is used in both Book and Page classes, it shouldn't be duplicated.

Do you propose make compress_jpeg method static?

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

Hi,

Sorry, I missed with why you changed compress_jpeg - if the code is common to both page.vala and book.vala it should go into it's own module (e.g. jpeg.vala).

However, while this bug does fix the DPI not being set it does mean we can't use the ICC profile support without re-implementing that too (it was commented out but it is supported in gdk-pixbuf). I meant to make a patch to gdk-pixbuf ages ago but I forgot - I've made this now for DPI [1].

I think that once the DPI patch is accepted to gdk-pixbuf we should just use that - it will be simpler for simple-scan without the jpeg code being accessed directly.

Sorry again, I should have picked this up earlier.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=701622

review: Disapprove
Revision history for this message
Victor Mireyev (greymouse2005) wrote :

Thanks you so much for fixing this issue and sharing common code with gdk-pixbuf!

Unmerged revisions

598. By Victor Mireyev

Remove JpegWriter. New DensityUnit enumeration

597. By Victor Mireyev

Save DPI with JPEG.

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