Patch #1271

Update printStructure

Added by Ben Touchette 3 months ago. Updated about 1 month ago.

Status:ClosedStart date:19 Jan 2017
Priority:NormalDue date:
Assignee:Ben Touchette% Done:

100%

Category:image formatEstimated time:40.00 hours
Target version:0.26

Description

Update patch to add empty pritnStructure to all image types. Also adds full (preliminary) printStructure support for RIFF and ORF file formats.

ps.diff Magnifier - adds more printstructureSupport (28.5 KB) Ben Touchette, 19 Jan 2017 14:25

patch_crw.diff Magnifier (871 Bytes) Ben Touchette, 31 Jan 2017 17:47

printStructure-3.diff Magnifier - This patch supersedes the previous ones. (20.3 KB) Ben Touchette, 15 Feb 2017 15:31

Associated revisions

Revision 4702
Added by Robin Mills 3 months ago

#1271 CMake support for FreeBSD. Requires -DEXIV2_ENABLE_NLS=OFF

Revision 4736
Added by Robin Mills about 1 month ago

#1271 Thanks to Ben for reporting this and providing the patch.

History

#1 Updated by Ben Touchette 3 months ago

Workaround for crashes with crw files.

#2 Updated by Robin Mills 2 months ago

  • Category set to image format
  • Status changed from New to Assigned
  • Assignee set to Robin Mills
  • Target version set to 0.26

#3 Updated by Robin Mills 2 months ago

Ben: I'm puzzled by this patch. There are two different subjects being mixed together:

1) Adding new stubs for printStructure for currently unsupported image types.
Is there something wrong with the existing behaviour of throwing an exception for unsupported file formats?
This feature was initially added for JPEG and PNG (by Tuan in GSoC 2013 in his version of exifprint.cpp
I moved it to $ exiv2 -pS foo.jpg and then added TIFF support in v0.25
I added more formats and the recursive version: $ exiv2 -pR foo.jpg in v0.26

2 Adding LocalNetFileIo into BasicIo
I would like to see this logged as a different issue.
Did you intend to put this into the patch?
I'm willing to split the patch and open and issue about LocalNetFileIo

#4 Updated by Ben Touchette 2 months ago

1) Is there something wrong with the exceptions; not from my point of view, but at the time i was asked to add them in by a client.There was a misunderstanding about what printStructure really did. I submitted it as requested by them. I have added on their end some changes that i need to fold back here. I guess the stub ones could be removed at this point in time. I may redo this soon to remove them and add the new full
implementations i've added on their end.

2) I have a separate class for blockfileio with uri for paths i did for them i'll hopefully be able to submit soon. I had to use a uri style entry (does not use a real uri since i pass it the real full path) to hook it up like http io does.

#5 Updated by Robin Mills 2 months ago

I'm lost. Am I to submit ps.diff with the Format::printStructure() methods AND LocalNetFileIo class, or will you provide a new patch?

#6 Updated by Ben Touchette 2 months ago

I'll make a new patch for this issue to supersede the two previous ones. I think i leaked some wrong code in that first patch that was from my first attempt at using remoteio. I need to review this once i've had enough coffee and breakfast. :)

#7 Updated by Robin Mills 2 months ago

Very good. Can you make the patch with the command:

svn diff
When you send the patch with the git commands, I have to edit the patch to get SmartSVN to accept it!

#8 Updated by Ben Touchette 2 months ago

Sure, does it accept regular "diff -u" files?

#9 Updated by Robin Mills 2 months ago

The correct format is like this:

514 rmills@rmillsmbp:~/gnu/exiv2/trunk $ svn diff .
Index: src/version.cpp
===================================================================
--- src/version.cpp    (revision 4705)
+++ src/version.cpp    (working copy)
@@ -2,7 +2,7 @@
 /*
  * Copyright (C) 2004-2015 Andreas Huggel <ahuggel@gmx.net>
  *
- * This program is part of the Exiv2 distribution.
+ * This program is part of the Exiv2 distribution you know.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
515 rmills@rmillsmbp:~/gnu/exiv2/trunk $ 
Your files are like this:
Index: src/tgaimage.cpp
===================================================================
diff --git a/trunk/src/tgaimage.cpp b/trunk/src/tgaimage.cpp
--- a/trunk/src/tgaimage.cpp    (revision 4698)
+++ b/trunk/src/tgaimage.cpp    (working copy)
@@ -74,6 +74,16 @@
         throw(Error(32, "Image comment", "TGA"));
     }

+    void TgaImage::printStructure(std::ostream& out, PrintStructureOption option, int depth) {
+        if (io_->open() != 0) throw Error(9, io_->path(), strError());
+        IoCloser closer(*io_);
So I have to edit your header from:
Index: src/tgaimage.cpp
===================================================================
--- a/trunk/src/tgaimage.cpp    (revision 4698)
+++ b/trunk/src/tgaimage.cpp    (working copy)
To:
Index: src/tgaimage.cpp
===================================================================
--- src/tgaimage.cpp    (revision 4698)
+++ src/tgaimage.cpp    (working copy)
So, I have to:

  1. Remove the diff --git a/trunk/src/tgaimage.cpp b/trunk/src/tgaimage.cpp command
  2. Edit a/trunk/src/tgaimage.cpp into src/tgaimage.cpp
  3. Edit b/trunk/src/tgaimage.cpp into src/tgaimage.cpp

I could write a little script to do this. However, I don't know what are a and b? Does git always do this, or does 'a' and 'b' mean something on your local file system.

I don't mind doing this in a little patch (to modify one or two files). However for a patch which changes many files (such as ps.diff), there are 31 files in the patch and it's very tedious to edit all of this manually.

#10 Updated by Ben Touchette 2 months ago

Here are the updates required by my client, some of them update tiff based formats, and several add some new ones for same tiff based format, and some are completely new as i needed them to debug some issues. The new tiff ones were added to make blockfileio work properly. Hoping to submit that patch soon'ish.

This patch supersedes the previous ones.

#11 Updated by Robin Mills 2 months ago

Do you want me to review and submit. Or only to review this patch?

#12 Updated by Ben Touchette 2 months ago

At least a quick review, if you want to submit sure, else i can do it after that.

#13 Updated by Robin Mills 2 months ago

Fine. I'll do that after dinner when Alison goes to the Scottish Country Dancing. I'll also hope finish to my work on the temporary file issue this evening.

#14 Updated by Robin Mills about 1 month ago

  • Status changed from Assigned to Closed
  • Assignee changed from Robin Mills to Ben Touchette
  • % Done changed from 0 to 100
  • Estimated time set to 40.00

r4735 Thanks to Ben for reporting this and providing the patch.

This is very good work, Ben. Thank You very much. I'm closing this and setting the "Estimated time" at 40 hours. You're welcome to modify that to reflect the effort involved.

Also available in: Atom PDF

Redmine Appliance - Powered by TurnKey Linux