Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

load and saveAs functions do not work as expected in a Linux environment #1

Open
simoninns opened this issue Jan 18, 2019 · 3 comments

Comments

@simoninns
Copy link

In JsonWax.h line 184:

qfile.setFileName( PROGRAM_PATH + '/' + fileName);

This is assuming that the directory separator is '/' which is only true for the Windows environment (same for line 247). It should be possible to use QDir to convert absolute and relative paths in a portable manner.

This causes the 'load' and 'saveAs' methods to fail unless an absolute path is passed (tested on Ubuntu 18.04 LTS).

(thanks for the GPL library btw :) )

@doublejim
Copy link
Owner

doublejim commented Jan 18, 2019

Hi, and thanks for your bug report, and for using this library!

This is an unexpected issue, as I have tested it on both Windows and Linux already. You should be able to solve the problem by replacing the '/' with QDir::separator(), if this really is the problem. Let me know if it works. And btw. '\' is the directory separator on Windows, but '/' works there too.

I should really look into updating this library, as I have done a lot of work to improve performance, which hasn't yet been shared (it then actually competes with the performance of QJsonDocument!). I mainly need to rework the code for the serializer now, and update the documentation. I've made bugfixes and improved the structure, and I've made it work on Windows XP, haha (that actually improved the code quality, though).

@simoninns
Copy link
Author

simoninns commented Jan 18, 2019

I looked into it a little more after raising the issue and found that the code was using the full-path but repeating the filename part twice ending up with /this/is/the/path/file.json/file.json so my quick assumption about the issue is probably wrong as you point out. It also prefixed the application path to the filename in certain cases (which for an installed binary was /usr/local/bin on my system) - so it might have something to do with my binary being 'installed' as opposed to running locally co-located with the target JSON file.

Changing the code to the following cured the issue:

if (dir.isRelative())
            // Note: Temporary fix for Linux
            qfile.setFileName( fileName);
        else
qfile.setFileName( fileName);

The calling code simply takes the filename as a string from the command line; the symptom was that the JsonWax library only opened the file if the passed string was an absolute path and filename ('./filename' or just 'filename' didn't work). I haven't tested the code on Windows yet though, so it may be a bad fix :)

Glad to hear you have some improvements in the pipeline - the Qt library has a weird limit of 128Mbytes on the document size making it basically unusable, hence my search for an alternative. GPL is a preference for me, so your library was perfect!

@doublejim
Copy link
Owner

The problem is the qApp->applicationDirPath() that runs in the constructor.

It says in the documentation that:
Warning: On Linux, this function will try to get the path from the /proc file system. If that fails, it assumes that argv[0] contains the absolute file name of the executable. The function also assumes that the current directory has not been changed by the application.

So, when running your program it uses the first argument, which is the filename to the json file you want to load, rather than the directory of the executable, as I expected.

Instead of setting the application path that way, I should've just let it be the current (working) directory. QFile allows for both relative and absolute filenames anyway, so I don't even need to check what the current directory is.

So, your fix is the proper way to go: qfile.setFileName(fileName);
However, you don't need to test whether the path is relative or not, since you set the filename to the same either way :)
And we can get rid of the qApp->applicationDirPath() part.

I will make an update with a fix within a couple of weeks (I want to make a proper library update).

Again, thanks for reporting the problem!

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

No branches or pull requests

2 participants