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

New instance improvements #653

Closed
wants to merge 5 commits into from
Closed

New instance improvements #653

wants to merge 5 commits into from

Conversation

zhekov
Copy link
Member

@zhekov zhekov commented Sep 10, 2015

This is an improved version of PR #637.

No more "--" - not needed.
No guard for is_osx_bundle() under Windows - does not belong here.
Split into proper sequential commits.
Fixed the encoding of options and doc_name.
Fixed the OSX bundle executable name.
Created a function to return the reproduced arguments instead of exposing global variables.
Re-tested under Windows and Linux.

There is only one problem left. As described in PR #637, under Windows, mscvrt often breaks unquoted locale strings on 2+ pieces, considering some characters "spaces". So spawn should quote any argv elements containing locale. But that must obviously be a separate PR.

main_get_persistent_argv() returns the (reproduced) options which
should be passed to a new window.
Since the executable is not a directory, renamed utils_resource_dir()
to the more generic utils_resource_path(), and RESOURCE_DIR_DATA to
RESOURCE_PATH_DATA_DIR etc.
Windows only: also fix the locale filename support. The standard
utils_get_locale_from_utf8() does nothing, because it targets the glib
filename encoding, which is UTF-8. g_locale_from_utf8() must be used.
@zhekov
Copy link
Member Author

zhekov commented Sep 10, 2015

Now, a few words about app->confdir under *nix.

Geany assigns "app->confdir = alternate_config", and then in setup_config_dir() does "SETPTR(app->configdir, utils_get_locale_from_utf8(app->configdir))", freeing app->confdir, and thus alternate_config, leading to buggy reproduced option for New Window.

The original SM code counters this by "app->confdir = g_strdup(alternate_config)", but in my tests with a -c locale_dir under Linux:

  • alternate_config was in locale (identical to locale_dir)
  • utils_get_locale_from_utf8(app->confdir) /from SETPTR/ failed to convert it to locale, and returned a copy of it
  • Help -> Debug Messages aborted Geany with an assertion that the text to be displayed (the 3 debug messages containing confdir, I checked that) is not valid UTF-8
  • The above points are compliant (if that's the word) with the GLib spec, which states that the G_OPTION_ARG_FILENAME options are in the GLib filename encoding, and that is locale under POSIX, not UTF-8.

It seems to me then that the proper fix will be (a) not to convert app->configdir to locale, and (b) use an UTF-8 converted string for geany_debug(). I tried that, and it works on my system (bg_BG). So my questions are:

  • Which way should I fix alternate_config being freed? Can you provide an example when converting app->configdir to locale makes sense?
  • If I'm to fix it by not converting app->confdir to locale, should I also include a fix for the Debug Messages?..

@zhekov
Copy link
Member Author

zhekov commented Sep 11, 2015

Hmm, there's one more bug: Geany executable is checked for being valid each time utils_resource_path() is invoked. I'll fix this soon. It's in the last commit, is it OK to replace it?

@zhekov
Copy link
Member Author

zhekov commented Sep 11, 2015

Fixed Geany executable to be checked only once. The bug wasn't in the last commit, so I'm not force-replacing it, but issuing a new one.

@zhekov
Copy link
Member Author

zhekov commented Sep 13, 2015

Since app->confdir is usually initialized with the dynamically allocated utils_get_user_config_dir(), and is gfree-d, using g_strdup(alternate_config) seems ok, and the confdir locale issue should be fixed separatedly.

@zhekov
Copy link
Member Author

zhekov commented Sep 18, 2015

Fixed configdir encoding in PR #658

@codebrainz
Copy link
Member

Closing since this hasn't been touched in almost 4 years and AFAIK @zhekov is no longer using Geany as his main editor. Feel free to re-open if anyone wants to pick up working on this.

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

Successfully merging this pull request may close these issues.

None yet

2 participants