Skip to content

Conversation

@nhz2
Copy link
Contributor

@nhz2 nhz2 commented Jun 23, 2023

Fixes #14 by using ZipArchives.jl

ZipArchives.jl is a new package I made to read Zarr data, and it works for PPTX as well.

Lastly, in this PR I removed the ability to use template directories, only files are allowed.

@nhz2 nhz2 marked this pull request as ready for review June 25, 2023 15:01
@matthijscox-asml
Copy link
Member

You put some serious effort into this!

I don't know if @jvieyras and/or @xvries have some time to review as well?

@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Attention: Patch coverage is 97.40260% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 95.84%. Comparing base (1f1adda) to head (4f175f1).
Report is 1 commits behind head on main.

❗ Current head 4f175f1 differs from pull request most recent head 2a195f3. Consider uploading reports for the commit 2a195f3 to get more accurate results

Files Patch % Lines
src/write.jl 96.87% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
+ Coverage   94.62%   95.84%   +1.21%     
==========================================
  Files          10       10              
  Lines         521      505      -16     
==========================================
- Hits          493      484       -9     
+ Misses         28       21       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nhz2 nhz2 marked this pull request as draft August 27, 2023 17:18
@nhz2 nhz2 marked this pull request as ready for review August 27, 2023 17:52
@nhz2
Copy link
Contributor Author

nhz2 commented Aug 27, 2023

I updated this PR to the latest version of main.

@matthijscox-asml @jaakkor2 This PR also resolves the problems in #47. With this PR there is no need to mess with file permissions or directories because everything stays as zip files.

@matthijscox-asml
Copy link
Member

@matthijscox-asml @jaakkor2 This PR also resolves the problems in #47. With this PR there is no need to mess with file permissions or directories because everything stays as zip files.

That would be another good reason to go with your changes :)

But you do not expect issues with editing the zip files, if the zip file is read-only from the Pkg.add ?

@nhz2
Copy link
Contributor Author

nhz2 commented Aug 28, 2023

In this PR the only thing I do with the template file is read, I never use the cp function.

To avoid changing an existing zip file, I keep track of two separate zip file objects when writing the presentation.

One is template_reader = ZipBufferReader(read(template_path))
This is a read-only ZipReader of the template.

The other is ZipWriter(temp_out; own_io=true) do w
This is an append-only ZipWriter for the new file.

w starts out completely empty, and files that need to be different from the template are added to it in:

update_presentation_state!(p, template_reader)
write_relationships!(w, p)
write_presentation!(w, p)
write_slides!(w, p, template_reader)
write_shapes!(w, p)
update_table_style!(w, template_reader)
add_contenttypes!(w, template_reader)

Then only after all of the files that needed to be edited were written, do I copy over all other files from the template that are not yet in w.

# copy over any files from the template
# but don't overwrite any files in w
for i in zip_nentries(template_reader):-1:1
    local name = zip_name(template_reader, i)
    if !endswith(name,"/")
        if !zip_name_collision(w, name)
            local compress = zip_iscompressed(template_reader, i)
            zip_data = zip_readentry(template_reader, i)
            zip_newfile(w, name; compress)
            write(w, zip_data)
            zip_commitfile(w)
        end
    end
end

In this way no zip file entry needs to be edited or deleted, only read and append operations are needed.

@matthijscox-asml
Copy link
Member

okay, i agree this looks good! hmm, I've decided to go with it.

do you want to do a release 1.0 of ZipArchives.jl first before merging this?

@nhz2
Copy link
Contributor Author

nhz2 commented Sep 4, 2023

Sure, I'll make sure to do that. Thanks for reviewing this.

nhz2 and others added 3 commits September 4, 2023 12:29
I removed some functions from ZipArchives.jl in JuliaIO/ZipArchives.jl#43 that are not needed by PPTX.jl to reduce the complexity. No changes to PPTX.jl should be needed.
@matthijscox-asml
Copy link
Member

I was ill for a while so this took a bit longer than expected.

I see that there's a silly error in the docs test that needs a fix (although I don't know why it suddenly needs DataFrames.DataFrame instead of DataFrame, but it's easy enough to change that)

If you want you can also update the Project.toml and bump it to v0.7.0, then I'll register a new version after merging this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Switch to using ZipFile.jl

2 participants