Fix/image order #497

knzwats · 2019-03-01T07:21:28Z

Summary

If there is a sheet without any images attached, indices for image files are messed up and crammed.

I had to have a proper sheet indices as a hash and later refer to it to correctly resolve the sheet that the image belongs.

My code might look wonky but it works!

here's the rake test result:

Finished in 9.867465s, 16.9243 runs/s, 222.4482 assertions/s.

167 runs, 2195 assertions, 0 failures, 0 errors, 7 skips

You have skipped tests. Run with --verbose for details.
Coverage report generated for Unit Tests to /Users/atsushikanazawa/Projects/roo/coverage. 1656 / 1898 LOC (87.25%) covered.

my evnviroinments are:

System Version: macOS 10.14 (18A391)
Kernel Version: Darwin 18.0.0

ruby version:

ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-darwin18]

Microsoft excel version:

Version 16.22 (190211)

Other Information

I attached an excel file that contains 2 kangaroos on sheet 1 and 3.
If you run it with older code, it tells you that sheet 1 and 2 have kangaroos.

…t any images attached in-between

chopraanmol1

Thank you for opening this PR. I think we should handle mapping logic in Roo::Excelx::Sheet and Roo::Excelx::Relationships.

Roo::Excelx::Relationships : Add new method to filter based on type. Similar to

roo/lib/roo/excelx/relationships.rb

Lines 16 to 20 in 4ec1104

    
           def include_type?(type) 
        
             to_a.any? do |_, rel| 
        
               rel["Type"]&.include? type 
        
             end 
        
           end

Roo::Excelx::Sheet : Add images method to lazy load following

roo/lib/roo/excelx/sheet.rb

Line 15 in 4ec1104

@images = Images.new(image_rels[sheet_index]).list

while retreiving mapping details from @rels
Move kangaroos.xlsx file to test/files folder. As we ignore test/files while building gem to reduce size.

roo/roo.gemspec

Line 17 in 4ec1104

spec.files.reject! { |fn| fn.include?('test/files') }

chopraanmol1 · 2019-03-01T10:52:23Z

test/image_order/test_image_order.rb

@@ -0,0 +1,11 @@
+require "test_helper"
+require 'pry'


Remove this line

knzwats · 2019-03-01T10:59:43Z

will do, thanks!

- created a method that returns "Target" attribute based on passed type - created "images" method on sheet class, that resolves the correct index by said "Target" attribute and regex - moved excel file to test/files - changed test file name because this is about indices of images, not ordering

knzwats · 2019-03-01T18:59:43Z

I fixed the code!

But I want to elaborate test cases a little bit more when I get to office(I don't have Excel installed on any of my cumputers thus cannot modify excel files at home).

Any test scenarios do you have in your mind?
Thanks!

coveralls · 2019-03-01T20:27:35Z

Coverage increased (+0.05%) to 94.418% when pulling a16ab56 on knzwats:fix/image_order into 4ec1104 on roo-rb:master.

lib/roo/excelx/relationships.rb

chopraanmol1 · 2019-03-13T09:04:53Z

lib/roo/excelx/relationships.rb

@@ -19,6 +19,12 @@ def include_type?(type)
        end
      end

+      def target(type)


target is a confusing name for what this method is doing.

We also need relevant test cases for the newly added method.

The method name should signify that we are returning target(s) while filtering relations on Type attribute. If it doesn't signify the above then it is confusing in nature.

chopraanmol1 · 2019-03-13T09:08:26Z

lib/roo/excelx/sheet.rb

@@ -96,6 +93,16 @@ def dimensions
        @sheet.dimensions
      end

+      def images


This method should be memoized.

def images @images ||= begin ... end end

chopraanmol1 · 2019-03-13T09:12:09Z

lib/roo/excelx/sheet.rb

+      def images
+        target = @rels.target("drawing")
+        match = /[a-zA-Z]+([0-9]+).xml/.match target
+        if match


Do we need this condition? Images class already handles nil as file_path

chopraanmol1 · 2019-03-13T09:15:47Z

lib/roo/excelx.rb

@@ -455,7 +455,6 @@ def process_zipfile_entries(entries)
          nr = Regexp.last_match[1].to_i
          image_rels[nr - 1] = "#{@tmpdir}/roo_image_rels#{nr}"
        end
-


Revert unnecessary cosmetic changes

sorry about that

- sort the images hash because its order was messed up too - added new test case which tests the said order

knzwats · 2019-03-14T09:22:32Z

Fixed!

But I have few questions.

I just noticed the order of image resources are like this if you have multiple images:

{"rId3"=>"roo_media_image3.jpeg", "rId2"=>"roo_media_image2.jpeg", "rId1"=>"roo_media_image1.jpeg", "rId4"=>"roo_media_image4.jpeg"}

It looks like it's random and I thought we'd better sort it by its number, so I coded just so it does.
Sorting a hash does not make much sense, but sheet#images method is only used inside excelx#images as far as I grepped, and keys are discarded so it is effectively an array, because keys are not exposed anyway:

def images(sheet = nil)
  images_names = sheet_for(sheet).images.map(&:last)
  images_names.map { |iname| image_files.find { |ifile| ifile[iname] } }
end

I bumped into this problem in my job and sorted images based on its names outside of roo.
Do you think sorting is necessary?

And I'd love to write some tests on relationship#filter_by_type method.
Should I add new test method to roo/excelx/relationships_spec.rb or create a brand new testcase?
Is there any excelx file or specific scenario to test relationship#filter_by_type in your mind because I do not know roo's internal design or its edge cases.

Thanks!
@chopraanmol1

knzwats · 2019-04-02T10:56:22Z

hello?

simonoff · 2020-08-18T12:27:43Z

lib/roo/excelx/sheet.rb

+          @rels.filter_by_type(type: "drawing").map do |target|
+            match = /[a-zA-Z]+([0-9]+).xml/.match target
+            Images.new(image_rels[match[1].to_i - 1]).list
+          end.reduce({}, :merge).sort.to_h


What if it fails?

knzwats added 4 commits March 1, 2019 16:09

fix a bug that image indices are messed up if there is a sheet withou…

05f5730

…t any images attached in-between

fuck me

0d8d777

fuck me

5fc479a

fuck me

e0f8bf9

chopraanmol1 requested changes Mar 1, 2019

View reviewed changes

chopraanmol1 reviewed Mar 1, 2019

View reviewed changes

chopraanmol1 requested changes Mar 13, 2019

View reviewed changes

code review

9aa08af

knzwats changed the title ~~Fix/image order~~ [WIP]Fix/image order Mar 14, 2019

- I had to reduce the array of hashes

46a4280

- sort the images hash because its order was messed up too - added new test case which tests the said order

knzwats changed the title ~~[WIP]Fix/image order~~ Fix/image order Mar 14, 2019

changed method name

a16ab56

simonoff requested changes Aug 18, 2020

View reviewed changes

	def include_type?(type)
	to_a.any? do \|_, rel\|
	rel["Type"]&.include? type
	end
	end

Fix/image order #497

Are you sure you want to change the base?

Fix/image order #497

Uh oh!

Conversation

knzwats commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Other Information

Uh oh!

chopraanmol1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knzwats commented Mar 1, 2019

Uh oh!

knzwats commented Mar 1, 2019

Uh oh!

coveralls commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knzwats commented Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knzwats commented Apr 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knzwats commented Mar 1, 2019 •

edited

Loading

coveralls commented Mar 1, 2019 •

edited

Loading

knzwats commented Mar 14, 2019 •

edited

Loading