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

Implement initial support for XDG [1] #988

Merged
1 commit merged into from
Nov 22, 2013
Merged

Implement initial support for XDG [1] #988

1 commit merged into from
Nov 22, 2013

Conversation

holomorph
Copy link
Contributor

I only used Config as I do not know what everything that calls PfaEditDir actually is, so I did not try to decide where each file belongs. I replaced the HomeDir function and avoided using existing GFile functions, as they use copy() which leaks.

Also, the files will not be created if XDG_CONFIG_HOME or ~/.config does not exist. Something in ffensuredir would fix that. Finally, because I have replaced getPfaEditDir completely, consideration for the VMS flag and renaming of ~/.PfaEdit is gone.
#847

Introduce getFontForgeUserDir(arg) where arg is one of Cache, Config, or
Data.  Replace every instance of getPfaEditDir(buffer) with
getFontForgeUserDir(Config).

[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
@ghost
Copy link

ghost commented Nov 22, 2013

After discussing it with @davelab6, I'm going to go ahead and merge this - it doesn't break the build - and then work on some improvements. I'm not so concerned about the use of exit(). That's only going to happen if malloc() fails while allocating space for the configuration directory pathname. I'll be amazed if you can actually cause that error to occur; checking for it is strictly a formality. Something that worries me more is the apparent memory leak, because the new function returns a character pointer that may be to something static or may be to something allocated with malloc(). The caller cannot safely free it, and nobody else will.

I do have a lot of qualms about style issues: indentation doesn't match the rest of the code base, no comments, the anonymous enum type should have a name and its values seem to be namespace-polluting, the PfaEditDir function seems like it should be deleted now that we don't want to use it, and so on. I'm also not sure what will happen if the directory doesn't exist - the former code handled that case and this code doesn't seem to. But I will work on some of those myself and file a pull request.

@ghost ghost merged commit e4c2cc7 into fontforge:master Nov 22, 2013
@holomorph
Copy link
Contributor Author

I have a bunch of comments to add, and I found a bug (s/buffer/dir/ in getUserhomeDir) so I'll probably add another commit to this branch if that's alright.

Another thing I think would be good is letting getFontForgeUserDir try to create the directory it's returning. Then all the functions that call it have to do is make their own directory or file inside what getFontForgeUserDir returns. I imagine mkdir_p would have to be implemented.

@ghost
Copy link

ghost commented Nov 22, 2013

I opened #991 with a list of things I think should be done. Let me know which of those you plan to do so we can avoid duplicating work.

I see in your latest pull you've changed the exit()s to NULL returns. That won't really fix the issue @JoesCat identified, since crashing with a null pointer exception isn't going to be any less mystifying to the user than crashing with a non-zero return value. However, I really think it's a red herring. Nobody's going to run out of memory on this particular call, and it's not worth the large amount of infrastructure that it would take to gracefully handle out of memory errors in this particular hypothetical case. Not when we have so many crashes and incorrect behaviours elsewhere in the package that are really biting people on a daily basis.

@holomorph
Copy link
Contributor Author

Since most everything that calls getFontForgeUserDir/getPfaEditDir checks for that return, I figured it made some sense to at least mimic what was already in place: exit is pretty ungraceful too, and I have no idea how NULL returns are handled two or three functions up.

@JoesCat
Copy link
Contributor

JoesCat commented Nov 23, 2013

@mskala - I somewhat agree with what you say, but sometimes you just got to fix something and leave the bigger things for later. It's a bit frustrating to try fix some bigger routines when the routines you call aren't "solid" and can potentially breakdown on you. Plenty of examples can be seen in "github fontforge issues", like ufo.c for example which calls other routines, and other similar "large" functions.

If modules are small-enough, you can do everything you want to do in one pass, but when modules get big, you've then got soo much stuff intertwining that you can easily get distracted into other branches that you then don't finish-up your original stream of thought. Examples of smaller functions like gutils/gimageread_.c are about as big as "I'd" like to consider as "independent" modules and still manage to get everything done in one pass, even if that one-pass takes over a week to get done, but in the end, the results are fault-tolerant and clean-up after themselves if errors occur. I consider the lack of "issues" complaining about many of the gutils/gimageread_.c functions as a good sign that they are fairly robust.

@holomorph, if you follow a few of my last function edits, like Unicode/ustring.c:utf8_idpb(), you'll notice that not all the routines that call it/them take advantage of error checks....yet...(key word being yet). What matters is that the functionality is "now" there, so that if you (or someone else comes along to) improve that bigger routine, you can make it more fault tolerant because you've got these smaller robust routines handy now
.
@mskala - a good example of what needs to have exit() taken out is the python.c and scripting.c modules. One small error in a script (such as a spelling error) and Fontforge would bail-out through an exit() - and because of that method of programming, anyone who asks for a simple feature request such as "add python scripting to the gui" hasn't realized that that is really a monumental task at this point in time. Not because you can simply call python scripting, but because any simple error is going to have FontForge simply disappear off-of the screen if you are running it strictly in GUI mode. As a command-line program, you can live with that because you see it on the command line, but at the GUI level, having FontForge disappear off of the screen due to an exit() is about as bad as having it exit due to a seg fault - if anything is to cause a problem, at minimum, you'd like to see some sort of error message appear instead of having the program exit/disappear off of the screen.

@holomorph, thanks for taking the time to add commenting plus put some changes from using exit, to returning some sort of error back to the calling routine. Even if the returned errors aren't used yet, someone else may come along and improve it further. You have to measure improvements in weeks and months, not as in now and next hour. as long as you make it sturdy, re-entrant, fault-tolerant, return-back errors - you've covered the ideals, and eventually, those get used higher-up later when someone has time to improve a higher-up function.

@ghost
Copy link

ghost commented Nov 23, 2013

I'd much rather see that kind of attention paid to memory leaks.

@JoesCat
Copy link
Contributor

JoesCat commented Nov 23, 2013

...Sending you a Coverity email invitation... ;-P

...anyone else interested?

@ghost
Copy link

ghost commented Nov 23, 2013

Not for me. They demand that I agree not to publish any comparison of it to other products, and that's a dealbreaker.

@JoesCat
Copy link
Contributor

JoesCat commented Nov 23, 2013

Sorry about that - I wasn't aware you were using another scan tool.

This pull request was closed.
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

Successfully merging this pull request may close these issues.

None yet

2 participants