Sigil-Ebook / Sigil

Sigil is a multi-platform EPUB ebook editor
GNU General Public License v3.0
5.96k stars 578 forks source link

[Bug]: Adding images tries to add to CWD #709

Closed LinAGKar closed 1 year ago

LinAGKar commented 1 year ago

Bug Description

When trying to add an image to an epub, it doesn't add it. Instead it creates an empty "Images" directory in CWD, and adds a broken href attribute. This happens whether I use "add empty SVG" or "add existing image". For example, if I'm running Sigil from the the directory /home/linus, and I try to add image.svg to an epub, Sigil creates an empty directory at /home/linus/Images, adds <item id="image.svg" href="..//home/linus/Images/image.svg" media-type="image/svg+xml"/> to package.opf, and does not add any image file into the epub. In the book browser, an entry for the image shows up called /home/linus/Images/image.svg, but when I click at it it's show as empty, and it disappears when Sigil is restarted.

Platform (OS)

Linux

OS Version / Specifics

OpenSUSE Tumbleweed

What version of Sigil are you using?

1.9.20

Any backtraces or crash reports

No response

LinAGKar commented 1 year ago

It also affects stylesheets, and maybe other types of files. It does not happen if you already have any stylesheet/image/etc in the book.

kevinhendricks commented 1 year ago

can not recreate this on macOS but I will keep trying. What happens if you try with a png, gif, jpeg instead of a svg.

kevinhendricks commented 1 year ago

Sigil stores all epub files in the system temp /tmp folder. What have you set your tmp folder to be located.

kevinhendricks commented 1 year ago

Just tested this with "Add Existing" both a css stylesheet and an svg image and all works. Are you sure you have not played with Sigil's temp (/tmp) folder assignment in Sigil Preferences? Or at your Linux system level?

LinAGKar commented 1 year ago

The same thing happens for jpg and png.

The temp folder is set to <SIGIL_DEFAULT_TEMP_HOME> in the settings, and when I have books open I can see files being stored in /tmp/Sigil-* directories.

To reproduce it, I can:

  1. Launch Sigil, so it creates a default template epub
  2. Delete the default sgc-nav.css stylesheet
  3. Save
  4. Close sigil
  5. Open the ebook I just saved in Sigil
  6. Try to add a stylesheet or image
kevinhendricks commented 1 year ago

Just tested exactly this on macos with Sigil-1.9.20 and it all just works.

So whatever you are seeing is either specific to linux or to your build.

This bug did exist in a much earlier Sigil build but has long been fixed. Are you sure you are using Sigil-1.9.20? Official builds are done with Qt 5.12.9. What Qt version are you using?

Please screen shout the "About Sigil" dialog and upload it here.

Did you build from source? Or is this a distribution build? If so what distribution specific patches to Qt and or Sigil are you using?

I will try to get a linux VM build of Sigil on my mac tomorrow sometime to see if I can recreate this on Arch/Manjaro.

BeckyDTP commented 1 year ago

@kevinhendricks Unfortunately, I confirm :( I can reproduce it on Sigl 1.9.20m, Windows 10.

I know you were getting ready to release version 1.9.30, but it might be worth tracking down and fixing this bug beforehand.

I confirm that the bug is present in both the official version and my built one. sigil-issue-709

<?xml version="1.0" encoding="utf-8"?>
<package version="3.0" unique-identifier="BookId" xmlns="http://www.idpf.org/2007/opf">
  <metadata xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:opf="http://www.idpf.org/2007/opf">
    <dc:identifier id="BookId">urn:uuid:54db4bb4-9145-462a-acea-56f2c3e74fa4</dc:identifier>
    <dc:language>pl</dc:language>
    <dc:title>[Tutaj tytuł główny]</dc:title>
    <meta property="dcterms:modified">2023-03-12T16:58:44Z</meta>
    <meta content="1.9.20" name="Sigil version"/>
  </metadata>
  <manifest>
    <item id="Section0001.xhtml" href="t/Section0001.xhtml" media-type="application/xhtml+xml"/>
    <item id="nav.xhtml" href="nav.xhtml" media-type="application/xhtml+xml" properties="nav"/>
    <item id="epub.css" href="C%3A/Program%20Files/Sigil/Styles/epub.css" media-type="text/css"/>
  </manifest>
  <spine>
    <itemref idref="Section0001.xhtml"/>
    <itemref idref="nav.xhtml" linear="no"/>
  </spine>
</package>

Additional info:

dougmassay commented 1 year ago

So if I'm understanding correctly, it's only happening when there are no pre-existing files of the same media type? If the epub already has images or CSS files in it, new ones are added correctly, right?

If so, this bug could have been around for quite some time.

kevinhendricks commented 1 year ago

But I just updated my Manjaro vm to the very latest and built master against Qt 5.15.8 and I was able to add a new stylesheet with flying colours.

I started with and empty epub2 that has no css file to delete and used Add Existing to add a css stylesheet and it works like a charm for both Linux and MacoX.

So something about him starting with an epub3 and deleting the existing stylesheet before adding the new css stylesheet seems to be triggering things. Can you confirm that starting with an empty epub2 and using Add Existing to add a new css stylesheet works for you on Windows.

And I will start tracking down what happens when deleting the last css file.

I still can not recreate his original post about this not working with png, svg, and etc since no deleting of files is done in that case.

BeckyDTP commented 1 year ago

Not exactly. I can add images (gif, jpg, png, svg) without problem, but I can reproduce these steps for CSS file. I still work 99% with EPUB2 files and that's why I never noticed this problem, maybe it's related to EPUB3 only. If I find a free moment later – I will do more tests.

BeckyDTP commented 1 year ago

Can you confirm that starting with an empty epub2 and using Add Existing to add a new css stylesheet works for you on Windows.

Yes, EPUB2 works OK.

kevinhendricks commented 1 year ago

Great. Thanks for testing. I will start trying to track down what deleting the last css file is doing.

dougmassay commented 1 year ago

I can duplicate the issue on Arch Linux with the steps from the above post (epub3-only):

https://github.com/Sigil-Ebook/Sigil/issues/709#issuecomment-1465214624

LinAGKar commented 1 year ago

I can reproduce it both the version from the OpenSUSE Tumbleweed main repos and with the Flathub version. Both use Qt 5.15.8.

Screenshot_20230312_175933 Screenshot_20230312_180721

I too can confirm it only happens with EPUB3

it's only happening when there are no pre-existing files of the same media type

Not exactly, it doesn't have to be the same type. If the book contains a stylesheet, I can add images just fine. Only if the book contains neither stylesheets nor images is adding stylesheets and images broken (and font, audio, video, misc and .ncx files). XHTML files don't matter though.

I don't think it has anything to do with deleting per se, it could happen if the book never had any such files to begin with. What matters whether the book contained any files other other than .opf and .xhtml when you opened it.

dougmassay commented 1 year ago

Not exactly, it doesn't have to be the same type. If the book contains a stylesheet, I can add images just fine. Only if the book contains neither stylesheets nor images is adding stylesheets and images broken (and font, audio, video, misc and .ncx files). XHTML files don't matter though.

Got it. Thanks for clarifying.

kevinhendricks commented 1 year ago

Just pushed a fix for this behaviour to master to the longest common path routine in Utility.cpp that triggered when only 1 file type was present when loading an epub.

If you get a chance, please confirm this fix on Windows and/or Linux.

LinAGKar commented 1 year ago

I can confirm that that fixes it

dougmassay commented 1 year ago

Seems to fix it on Arch.

I just wanted to note that using the previous test epub that was saved in step 3 of https://github.com/Sigil-Ebook/Sigil/issues/709#issuecomment-1465214624 seemed at first to exhibit the same bad behavior. But somehow, when trying to delete the CWD path to the css file, I removed the file itself, but it was still manifested in the opf with the local file path. So whenever I went to add the css file, it used the existing manifest entry for the path. There was no problem at all when using a fresh epub to perform the test.

Firing up a Windows VM to test now, but I have a high degree of confidence it's fixed. Thanks Kevin! And thank you @LinAGKar for reporting the bug and providing steps to reproduce it. :)

dougmassay commented 1 year ago

That last push definitely fixes things on my Windows VMs

kevinhendricks commented 1 year ago

Same bug may exist in PageEdit version of that routine. Not sure if PageEdit uses absolute paths and Sigil does not. Will have to look closely at it.

kevinhendricks commented 1 year ago

As it turns out, PageEdit always uses absolute file paths with that routine so the PageEdit version of longestCommonPath is correct as it stands.