Old Paint Brushes Code

This isn’t a bug per se, just IMHO some fiendishly confusing code dating back to I think near the beginning.

When Paint loads, it has to pick a default built-in brush. It does this via a hard-coded string, “circle”. Due to localisation requirements, the sprite name for this brush may differ from this. They solved this by adding a line to Messages with the token “circle” returning the value “Circle” for English-speaking users.

In itself, this all makes perfect sense.

Now, let’s take a look at how it works in the code.

First the Messages token for the default brush is hard-coded as follows:

/* SMC: The following are not sprite names. They are tokens which must be
 * used to look up the real sprite name in the Messages file.
 */
char        tools_brushsprite [13]  = "circle";
char        tools_newbrushsprite [13]  = "circle";

Later, the tool window’s brush Name writeable icon is created:

    /* SMC: Look up default brush name in Messages file. */
    tools_icons [tools_bicon_NewName] = createicon_ind(12, 0, msgs_lookup (tools_newbrushsprite), 13, (char *)-1);

That’s where things become, IMO, very counter-intuitive. You see, tools_newbrushsprite contains the “circle” token at this point, but when msgs_lookup() finds that token in the Messages file (which it invariably does), it actually returns a pointer to the corresponding value inside its own internal dictionary!

createicon_ind() makes an icon with indirected text, so the end result is that, provided “circle” is in Messages, RISC_OSLib’s internal msgs_lookup dictionary entry will get used to store the writeable icon’s text from then on; it won’t be stored in the tools_newbrushsprite string!

This counter-intuitive string location doesn’t stop Paint working, because whenever the brush tool’s OK button is clicked, a msgs_lookup() is performed again:

  /* SMC: Look up default brush name in Messages file. */
  if ((newbrushsprite_ptr = psprite_find (msgs_lookup (tools_newbrushsprite), "PntE5a"))
      == NULL)
    return;

At first glance you’d expect tools_newbrushsprite would have the brush name the user typed in. It doesn’t. It always contains what it was initialised with, “circle”. Luckily, as before, msgs_lookup(“circle”) returns the address of the same string in the Messages dictionary which does now contain the user input!

My take on this is that we never want to perform a Messages lookup on brush names the user typed (I don’t know if that was ever wanted, it certainly wasn’t happening anyway! Although it would incidentally if someone deleted “circle” from a Messages file), because the user types sprite names and the sprite names should already be in the user’s local language. For that reason I’m tempted to turn “circle” into a #define constant, make the text box use an indirected string that Paint actually controls (which is what I’ve already done), and only ever do the Messages lookup on the rare occasion that there’s no valid brush selected which will be on startup and also if a user brush’s sprite got deleted (which is a bug Will Ling spotted).

I don’t really need feedback on this so don’t worry if it doesn’t make much sense. It was more me venting about something that took me a little while to figure out and a mental note to aid understanding of this bit of code in future.

In case anyone was wondering what the other string, tools_brushsprite, which is also initialised to “circle”, is for, it seems it was supposed to hold a working copy of the name of the currently selected brush, but in actual fact that wound up being forever “circle” too, so I may just remove that variable!

This whole thing doesn’t make any sense. The brush tool starts up with “Circle” by default. There is also “Square” and “Triangle”.

Now, does this mean that each language version of !Paint needs to have a different Sprites file with translated versions (cercle, carré, triangle; ciculo, cuadrado, triangulo; Kreis, Quadrat, Dreieck…)? That’s a bit nonsense, isn’t it, given that it’s only sticking messages into a Messages file, but not even bothering to put things into a country-specific subdirectory.

RISC_OSLib’s internal msgs_lookup dictionary entry will get used to store the writeable icon’s text from then on

Uh… are you saying it writes the user input over top of the library’s global Messages dictionary entry?

so I may just remove that variable!

Wouldn’t tools_brushsprite be where you’d want the brush sprite name stored?

There is also “Square” and “Triangle”.

Don’t forget the elliptical “Brush”. I have a feeling yonks ago that might have even been the default brush. I’m not sure.

Now, does this mean that each language version of !Paint needs to have a different Sprites file with translated versions (cercle, carré, triangle; ciculo, cuadrado, triangulo; Kreis, Quadrat, Dreieck…)?

Yup.

That’s a bit nonsense, isn’t it, given that it’s only sticking messages into a Messages file, but not even bothering to put things into a country-specific subdirectory.

It’s under “Resources.UK.Messages” beneath Paint’s source directory (along with Sprites and Templates), but the Disc build just copies Messages, Sprites and Templates into the application root directory. I imagine they might have done separate builds for different markets, though I’m just guessing.

Uh… are you saying it writes the user input over top of the library’s global Messages dictionary entry?

Uh huh. I think it’s only global within the current Paint application instance though.

Wouldn’t tools_brushsprite be where you’d want the brush sprite name stored?

Possibly, although in the pre-bounty Paint the “Name” field arguably was for specifying the name of a new brush sprite, so tools_newbrushsprite would have been logical (if that had actually been used properly for that, which it really wasn’t). The writeable icon’s even called tools_bicon_NewName.

I’m tempted to turn “circle” into a #define constant,

Actually that would be pointless. Messages tokens are normally string literals in the code. Something like msgs_lookup(“BRUSHDEFAULT”) will make it a lot clearer what it’s for and that the actual sprite name in Messages might be something other than “Circle”.

This whole thing doesn’t make any sense. The brush tool starts up with “Circle” by default. There is also “Square” and “Triangle”.

Just to clarify how pre-bounty Paint has always worked: if you type in “Square” or “sQuaRE” or “triangle”, those are taken directly as sprite names. They’re case insensitive, but sprites with those names need to exist for those brushes to be selected. There’s no localisation going on there except as you noted if Paint was shipped with non-English sprite names in Sprites.

For the default brush to be localised, the non-English word for a circle would need to be put in Messages under the “circle” token, with the circular brush sprite having that corresponding non-English name in Sprites.