Created attachment 230514 [details] [review]
When a font does not exist, default to default font.

In the given case, the crash was actually happening in app/widgets/gimptextbuffer.c - gimp_text_buffer_get_font_tag() with a font == NULL.
It would then strcmp() a string and a NULL pointer, which would segfault GIMP.

I was actually not sure what should be the expected behavior of the function if font is NULL (actually I am not sure what is the purpose of a font tag in GIMP), so I thought it would be better to fix before, the "why" it is set to NULL at the first place.

This problem happens when you select the text, it then tries to gimp_text_style_editor_set_default_font(editor) [app/widgets/gimptextstyleeditor.c]
=> gimp_context_set_font_name (editor->context, editor->text->font = "Garamond Italic");

This font name being non-existent on my system, the subsequent gimp_container_get_child_by_name (context->gimp->fonts, name) returns NULL.
What I did in such a case, is that I default to default font when this happens.
Cf. attached patch.

Note though that I wonder if this is the best fix to silently change the originally selected font. I think it could be better user experience to pop-up an error and tell the user <<you don't have the font "Garamond Italic" this text has been originally written with. We will default to font "XYZ". To obtain the expected image, you should install this font or select another similar>>. Or some error feedback like this.

Also side note: at first when I load the image in GIMP, the text appears exactly as the screenshot that the uploader provided (attachment 155852 [details]). Only when I actually touch the text will it either crash or suddenly change display font. Is there a preview image of text saved inside the XCF files?

Silently changing the font is A Bad Thing.  That leads to all manner of confusion when collaborating.

My 0.02.

Chris > I agree this is not optimal. My current patch at least prevents a GIMP crash (which is even worse). That's why I asked if we want to do more, we should agree on the best approach.

Is anyone ok with the warning pop-up when the defaulting font process happens, stating the name of the expected font, and the name of the default font? If yes, I'll make a patch this way. What should be the best wording?

If you have better approach, then anyone can propose, I guess. :-)

Created attachment 230674 [details] [review]
gimp_context_set_font_name() can fail. If it happens, cleanly default to default font, and warn the user.

Ok so attached is a nicer and more complete patch. Now gimp_context_set_font_name() has a new GError argument, which will be set in case of failure.
I handle such a failure in the different part where it is called in our call, and default the style editor to default font when this happens, while also gimp_message() the user (which usually means an informative popup in normal GUI mode, or else in console, etc.).

Where do you change the font to make is crash? See also bug 417704.
We should not set another font but rather simply avoid the crash.

Michael > in my comment 1, I gave the procedure with which I made it crash each and every time on master (was it what you asked?). Of course you must not have the font in the XCF (in the one above for instance, I have no Garamond Italic installed on my system).

I agree that the best is usually "simply" to not crash, but I don't see how it is possible in the current case. If someone tries to edit a XCF with a text and does not have a font, well one does not have it. There is no *good* solution here.

Which is why I think the solution I propose is the nicest (among which I can think right now), because we at least warn the user and even name the original font so that one can eventually research and install it if possible before going further. This was a problematic point in the bug 417704 that you linked as Amar Takhar points out (bug 417704, comment 3).

Or maybe I miss something. How would you deal with the case? We just prevent the text box modification until the font is installed?

Created attachment 231123 [details]
Clicking on the text

Created attachment 231125 [details]
updating the text

Created attachment 231126 [details]
Clicking on the text with patch applied

IMO gimp_message() is one of the worst solutions. Can't we have any indication in the UI of the text layer - the layers dialog, or on-canvas - that the fonts associated with this layer is missing?

Locking the layer might be a good idea, too.

To illustrate the issue and the patch, I uploaded 3 pictures:

1/ attachment 231123 [details] is what happens when you click on the text on git master. As you can see, the display is what is expected (cf. attachment 155852 [details]), so I imagine the image display is saved in the xcf. It also means that you can do any transformation (resize, cage, unified transformation tool, etc.) "as an image" as long as you don't actually edit the text.
The only proof of a problem in this screenshot is that the font slot is empty! It means also that the user has no idea what is the font name used if one does not have it.

2/ attachment 231125 [details] is what happen if you update the contents on git master. Here I added "XX" at some random point. It does not crash, but the font suddenly changes to something else (the default one) and there is no user feedback. In particular still no font name displayed (so now you know neither the original font nor the actual one).

3/ No screenshot necessary for it, but basically some other changes, like trying to change the font size for instance will segfault GIMP. So currently there are some changes (editing text contents) which simply have bad feedback, while other (font size for instance) just crash the program badly.

4/ attachment 231126 [details] finally is what happens when you click on the text on git master with my patch applied. Here I provide a nice feedback (in the form of a GIMP message) to the user telling one which font was originally used, which font it has been changed to instead, and as you can see the font slot is now filled with the actual font name.
So you can still make transformations using original font as long as you don't edit. And if you edit, well at least the program don't crash and you have a useful feedback allowing you to install the right font.

Michael Schumacher > locking the text layer indeed, and preventing from text edition is the other solution I thought of. Like you could still apply image transformation, but no text ones. This is also a feasible idea, except that I can easily imagine users to be frustrated if the original font is impossible to install (this is a proprietary font; or a custom font; or available on some system only; or paid one; or the original author of the XCF file is not available anymore; or some mix of these, etc.) for instance and you still want to edit the image.
If you got the file from someone long ago (or you did it long ago and your system changed), you want to reuse the exact same image but just modify a little the text, bam! you're blocked.

Also some fonts have been created specifically as alternatives of other, very close looking. A perfect example of these are the Liberation Fonts (http://en.wikipedia.org/wiki/Liberation_fonts) which were made on purpose to replace Windows fonts with close looking fonts and similar metrics. So for instance if I load a XCF and the font used is arial but I don't have it. Well I prefer the text layer to be editable so that I can switch to Liberation Sans with minor difference. If the layer is fully locked until I install the font, which I can't, well I'll be quite unhappy.

As for using the gimp_message(), I don't see why this is a bad idea. As far as I can see it has been created specifically to display warning and error. And this is exactly what this is. If we were to lock a layer or switch fonts as a "better-than-crashing" solution, I definitely want the user to understand very well how this happened, and not hide the information on some small distant area of the UI. This is the best idea to have angry users who come and say "hey what happens, my text layer is not editable/changed font!", whereas a clear warning pop-up is unmissable, and has a very clear description of the problem (which is a system problem that only the user can fix).

Let's assume we got a image with 20000 text layers, all using different combinations of non-existing fonts. How often do you want to show warning popups to the user?

How do you expect them to judge whether they can modify text in an image easily? This should be visible from even a cursory inspection of the layers dialog - if they see many highlighted (by whatever means) locked text layers there, they can tell that something is amiss.

In my opinion, the "small distant area" you're talking about is the error message: easily forgotten until the next time the users tries to use the text tool on another layer; whereas an indicator on-canvas or in the layers dialog would be much more obvious.

> Let's assume we got a image with 20000 text layers, all using different
> combinations of non-existing fonts. How often do you want to show warning
> popups to the user?

I don't show the popup at startup, so I won't be showing 20000 popups. The message is only shown when you select a text with a text tool (in other word, when you are going to edit a text, hence you need to know).
As a side note, this example is really not realistic. Who really makes 20000 text layers, all with various different fonts? But well, even with this nearly impossible case in real life, the message solution is perfectly ok and informative in my opinion.

> How do you expect them to judge whether they can modify text in an image
> easily? This should be visible from even a cursory inspection of the layers
> dialog - if they see many highlighted (by whatever means) locked text layers
> there, they can tell that something is amiss.

1/ In your 20000 different text layer example, are you telling me that we should load each of the 20000 text layers at image loading, just to load all the font and verify their existence? This is over-engineered. Your 20000-layer-text image will take minutes to load just to test all the fonts. Really the current implementation would load fonts when needed, and that's much better to keep it this way and it "scales" perfectly (even to the impractical 20000 layer case).

2/ Why would the user care to see all the font issue if he does not want to modify the text? This is over-informative! As I said, as long as you don't try to modify the text (contents, size, style, font...), you can do all the usual image transformations on your text layer which keeps the original non-existent font display all along. So there is no reason to block the user from doing so and to show 20000 errors in the UI from the start while one never even intend on editing the text contents/style anyway.
They don't want to "judge" anything.

> In my opinion, the "small distant area" you're talking about is the error
> message: easily forgotten until the next time the users tries to use the text
> tool on another layer; whereas an indicator on-canvas or in the layers dialog
> would be much more obvious.

Well that's the point. You don't want to overwhelm the user with errors for no reason when one can perfectly work with the image. You tell what's wrong when needed by the user, and not *force* this user to take action when not needed.

Created attachment 231273 [details] [review]
At style editor construction, bootstrap it with initial values.

I discovered another case where the same crash happens but the reason the font is not set is different.
Basically when the text style editor is "constructed", it sets all the events and callback to update style values when one of them is changed, but it does not bootstrap these values. So the first bad consequence is that if you just start GIMP, the first time you click with the text tool on some blank place, the displayed editor has no font/style/size displayed. The second bad consequence is that when you change for instance the size without setting the font or writing some text first (which would trigger the callback and fix the no-initialization issue), GIMP crashes the same way as the previous case.

The attached patch fixes this.
I did not integrate it in the same commits, because the reason is different so I consider them different bugs (though obviously linked).

Review of attachment 231273 [details] [review]:

Terricic, please push that one right away, to 2.8 and master,
but please add a blank line before.

"Terrific", even

Branch master
=============

commit 20c86f821df507e2067b892abd9cdd63950ebbed
Author: Jehan <jehan@girinstud.io>
Date:   Mon Dec 10 20:14:16 2012 +0900

    Bug 689523: data bootstrap initial values in the text style editor at construction.

    This was causing first a visual issue where the style editor UI would
    show no default font/size/style at instanciation, but even a crash
    when the user would change the font size or style (bold, italic...)
    from this UI before selecting a font or writing a text.


Branch gimp-2-8
===============

commit a2772a721fcceffdf99cc631c874ecdf2f75ee8a

I don't close the ticket because there is still the other case (which also makes a crash) where the font is not set because it is not installed that we still have to finish to discuss and decide. :-)

how about asking the user each time a font that is not installed is tried to be accessed:

'The font "FONT-NAME" used in this layer is not installed on your system. Would you like to change the font into the default font so you can edit it?'

'Yes, for only this layer'
'Yes, for this and future cases during this session'
'No, do nothing'

I guess the button text should be shorter, but the choices seem useful to me

Hi vaifrax,

that's a possibility but if we do so, there would still need a way to unblock the layer (or all other in case the user chose the all-session case). But if the user finally changed one's mind and wants to modify the text; or he chose to save the choice for all-session, but finally wants to modify another text in the same image, the only solution will be to restart GIMP. And we know that's a good solution for nothing (you lose your editing history, your work context, you break your work flow in the middle, etc.).

So that's an interesting proposition, to allow the user to either edit the text, or lock the layer, hence keep the font display (but not been able to change the text contents or style). You get the advantages of both world AND you get to ask the user, not impose one anything. Only it just needs to go a little further and give possibility to unblock later if you change your mind.

I have made a small test. The "lock" layer feature (which prevents from "drawing" in image layers) currently does not work for text layers (you can still edit your text after you clicked the lock button) currently. I will do a patch to make this lock feature work for text layers too. Then I could use this lock feature and try and implement your proposition.

Mitch > how does it sound? Do you have another proposition? We can't really let such a thing which can crash the program unattended, can we?

Actually, the context is and was always supposed to deal with objects
that are not available by using their name. This should fix the crash
in master and gimp-2-8. Sorry for not getting back here earlier, as a new
rule, nobody should be filing bugs in december ;)

commit 6a39d214947da4aba7354d25655065f9d42456c2
Author: Michael Natterer <mitch@gimp.org>
Date:   Fri Jan 18 01:02:31 2013 +0100

    Bug 689523 - GIMP crashes (segfault) when loading a context with a font...
    
    Make gimp_context_get|set_font_name() actually deal with
    context->font_name, so the context can do its job of keeping the name
    of an unavailable object around.
    (cherry picked from commit c262fee2445734c015aafe527cb681eed6941ec4)

 app/core/gimpcontext.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Michael: I just re-compiled master, and I re-open the ticket because it looks like the fix is incomplete. If I understand, with this patch you wanted the "standard" font to be used, but the expected original name to show.

Well I tested. There is indeed no crash anymore. And it uses the standard font when you start and edit the text. Nevertheless the font name is *always* showing blank only. So there is absolutely no way for the user to know what font is expected and search for it (even if GIMP itself keeps track of it internally).

Is that what you expected with this commit? If so, you can re-close, but I personally don't think this is all that satisfactory. But hey you're the boss!

Also even if we got this behavior (showing the expected font name, but using the standard one), it is sneaky and there is no explanation for the user. One may perfectly believe this is a bug: "hey that font changed all by itself!".

What about this: when the font name displayed is different from the font actually used, the font name is shown in red (and when you mouse over the red font name, the help bubble says something like "font unavailable on this system")?
Would this be acceptable? That's nearly same as what you propose, there is no popup, no layer lock, just a red fontname in the text editor to show something is amiss.

Note: though re-opened, all the crash parts being fixed, I change the importance from critical to normal.

I completely agree, but we have bug 417704 about empty font names
for missing fonts :)