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

Upgraded from 0.11 to 0.12, open from command line and dolphin broken #898

Closed
bame55 opened this issue Jul 1, 2021 · 2 comments · Fixed by #901
Closed

Upgraded from 0.11 to 0.12, open from command line and dolphin broken #898

bame55 opened this issue Jul 1, 2021 · 2 comments · Fixed by #901
Labels
Milestone

Comments

@bame55
Copy link

bame55 commented Jul 1, 2021

Environment: Ubuntu 20.04, Kernel 5.9.12, KDE 5.22.2, Qt 5.15.3

After upgrading from 0.11 to 0.12 (via download+dpkg) double-clicking icons in file manager (Dolphin) no longer opens msk projects. I can only open files from within the GUI.

Attempting to run via bash with an existing msk file yields the following:
$ manuskript p1.msk
usage: manuskript [-h] [--console] [-v] [-L LOGFILE] [FILENAME]
manuskript: error: unrecognized arguments: p1.msk

Tried the git version with the same result.

@worstje
Copy link
Contributor

worstje commented Jul 9, 2021

Regarding the argument error, I have reproduced this on Windows and identified the problem. It is completely my fault, sorry!

I am not sure how it slipped through, but it appears to have been an unfortunate combination of events. Obviously the filename was the first functionality I ever implemented, and I am sure it worked at the time.

Given that I forgot to adjust one line of the filename parsing code over from sys.argv[1] to the argparsed version, it seems obvious that calls that used to work continued to work by accident. But why args[1] instead of args[0], and why wasn't it a problem before?

When starting Manuskript, sys.argv resembles ['bin/manuskript', ...], which means argparse was consuming the script name as the filename parameter in the rewritten functionality... but it went completely unseen because it accidentally appeared to work still.

And when things stopped to work as work on the new logging facilities progressed, it was still likely to continue unseen because the functionality that autoloads the last project would have hidden it. (But even now it continues to go hidden, because all the argument parsing happens before the logging framework is set up... Now I want to add a line that at least logs we are loading an existing project file...)

Regardless, it is completely my fault for not catching this. Given the fact that I wrote and used this code for over a year before it got merged into manuskript without ever noticing the problem, it feels really damn stupid. Sorry!

As I am currently seeing how many little fixes I can create during my spare time this weekend, I'll end up creating a PR with the fix for this included soon.

@TheJackiMonster
Copy link
Collaborator

@worstje I will try to review the PR when it's ready as soon as possible to merge. So at least people can run the source from Github to fix this. I personally never noticed this because most of the time I just write on the same project which gets opened automatically when I start Manuskript. I guess, we could add tests for functionality like this to the CI later on. So it can't slip through.

worstje added a commit to worstje/manuskript that referenced this issue Jul 10, 2021
In addition to fixing the bug, related code that allowed this one to
slip under the radar has been cleaned up. Validation of the FILENAME
argument is now performed during parsing.
worstje added a commit to worstje/manuskript that referenced this issue Jul 10, 2021
In addition to fixing the bug, related code that allowed this one to
slip under the radar has been cleaned up. Validation of the FILENAME
argument is now performed during parsing.
@TheJackiMonster TheJackiMonster linked a pull request Jul 10, 2021 that will close this issue
@TheJackiMonster TheJackiMonster added this to the 0.13.0 milestone Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants