Migrate from popt to GOption

Bug #195220 reported by Daniel Macks
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
GLib
Fix Released
Medium
Inkscape
Fix Released
Wishlist
Krzysztof Kosinski
Inkscape Devlibs
Triaged
Undecided
Unassigned

Bug Description

inkscape presently (svn trunk) uses libpopt to process command-line flags. Most gnome-based packages have switched to using GOption (part of glib since version 2.6) for this task. Any thoughts about migrating inkscape to GOption? It would eliminate an external dependency (one that many/most users no longer "have installed anyway for some other package") and provide a more uniform user experience (help-screen, etc.).

Tags: cli
Revision history for this message
Krzysztof Kosinski (tweenk) wrote :

I would like to volunteer for that one. While I'm new to Glib programming, the project is well-defined in scope and requires learning only a small part of both Glib and Inkscape.

Revision history for this message
Tom Davidson (tjd-mit) wrote :

Gotta love it when someone steps up so quickly on a coding request! Chris I noticed you fon't have bits to set bug status, so I marked this one as 'in progress' for you. If it turns out that you can't work on it after all, then just leave another comment here and someone will set it back to 'unassigned'... Be sure to ask around on #inkscape for help, if you don't already hang out there--I have found the devs to be really glad to help new contributors find their way around the code!

Changed in inkscape:
assignee: nobody → tweenk
importance: Undecided → Wishlist
status: New → In Progress
Revision history for this message
Tom Davidson (tjd-mit) wrote :

Should be "noticed that you *don't* have bits". . . I wish you could edit your comments!

Revision history for this message
Krzysztof Kosinski (tweenk) wrote :

Migration completed. I'm not 100% sure about removing fixupSingleFilename(), replaceArgs() and related cruft, though I think it's no longer necessary, because Glib takes care of all that. I changed the LANG variable to a different encoding and everything seemed to be OK. However, I haven't tested on Windows.

Important changes:
- Many things refactored: killed sp_process_args(), sp_common_main() and the argument enum, removed the strcmp monstrosity that determined whether to use the GUI, etc.
- Standard GTK options are now accepted.
- Options put into groups.
- Moved print-something-and-exit cases from sp_process_args() to main()
- And of course removed popt dependency.

Regressions:
- Long option descriptions not aligned neatly like with popt. This is not yet implemented in Glib.
- Unsure about how filenames with non-ASCII characters will behave on Windows

Remaining work:
- Move command-line operations into a separate file.
- Move all option-related stuff, including the global variables, into a new class.
- Allow to work one more than one file in a single invocation, i.e. "inkscape --file=test.svg --export-pdf=a.pdf --export-eps=a.eps --file=test2.svg --export-pdf=b.pdf"
- Make --verb and --select options work while --without-gui (doesn't work in current SVN)

Revision history for this message
Krzysztof Kosinski (tweenk) wrote :

Ok, now I think I know what replaceArgs does, and it was wrong to remove it. New patch coming shortly.

Revision history for this message
Tom Davidson (tjd-mit) wrote :

Chris, just leave a note here when you are ready to have this committed to the development tree, or drop in to #inkscape on freenode.net, and ask someone to review it for you and commit it to SVN. The devs are all pretty busy with the 0.46 release right now (and we're past hard freeze, so your patch will most likely be headed for 0.47) so don't be too surprised if it take a few days to hear back...

Revision history for this message
Krzysztof Kosinski (tweenk) wrote :

After consulting some documentation I think that replaceArgs is necessary only if the filename supplied on the command line is not representable in the system character encoding, which is a pathological situation anyway. Otherwise, handing argc and argv to GOption should be OK.

The new patch fixes a typo that would cause a compilation error on Windows and cleans up some unnecessary includes.

Revision history for this message
Krzysztof Kosinski (tweenk) wrote :

I somehow forgot to mention that if nobody objects to removing replaceArgs() and trusting Glib instead, this patch should be ready to commit.

The "remaining work" items are just my suggestions on how the command-line interface can be improved further, but this should be worked on after the current patch is committed and tested for regressions.

Revision history for this message
Krzysztof Kosinski (tweenk) wrote :

Revisiting this concept after a long time... Implementing this depends on this Gnome bug getting fixed:
http://bugzilla.gnome.org/show_bug.cgi?id=522131
Otherwise we'll get broken filenames on Windows, and opening some files will fail.

Changed in inkscape:
status: In Progress → Confirmed
jazzynico (jazzynico)
tags: added: cli
jazzynico (jazzynico)
Changed in inkscape:
status: Confirmed → Triaged
Changed in inkscape-devlibs:
status: New → Triaged
Changed in glib:
importance: Unknown → Medium
status: Unknown → Fix Released
Revision history for this message
Alex Valavanis (valavanisalex) wrote :

Would be good to revisit this for 0.92, now that the Glib bug is fixed. I wonder though, whether we should be using Glib::OptionContext [1] as part of our C++ification drive.

@Krzysztof - any thoughts?

[1] https://developer.gnome.org/glibmm/2.42/classGlib_1_1OptionContext.html

Revision history for this message
Qantas94Heavy (qantas94heavy) wrote :

Just a couple of build/packaging files that still reference popt/libpopt -- once they're removed this will be done.

Changed in inkscape:
status: Triaged → In Progress
milestone: none → 1.0
Revision history for this message
Qantas94Heavy (qantas94heavy) wrote :
Revision history for this message
Qantas94Heavy (qantas94heavy) wrote :

All reference to popt have now been removed: https://gitlab.com/inkscape/inkscape/merge_requests/594

Changed in inkscape:
status: In Progress → Fix Committed
Max Gaukler (mgmax)
Changed in inkscape:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.