When the SDL User-Interface displays messages, it must "fold" long lines, so that they will fit within the message-box. Those lines are folded at spaces. If a "word" (characters between two spaces) is longer than the box is wide, then the current code cannot find a place to fold the text -- it gives an assertion error (which cancels the emulator). Usually, it happens when the UI tries to show long file path-names that have no spaces.
This patch makes the SDL code forcefully break and fold too-big "words" at the right edge of the box. Then, the code won't crash.
Thanks for the patch. Could give and example of where I could generate such an entry that breaks the dialog box? Perhaps some setting in (sdl-)vicerc?
Look at bug report #831. "Querino" saw that bug when he tried to load a snapshot file with a long path-name (longer that 28 characters). The error message mentions the path-name of the file that has the "difficult" header. (I guess that you would need to use SDL WinVICE, in order to see it.)
I am not aware of any other messages that can show the bug.
Applied in r32640, works fine. Thanks again for the patch.
yes, i can confirm that the "assertion error" is gone, i get the proper error message now (error while reading module header in snapshot) in bug #831 https://sourceforge.net/p/vice-emu/bugs/831/
"while (strlen(text) < ...)" is a horrible construct. A single call to determine the length should be enough.
The length changes on each pass through the loop. After a suitable display-line is measured, a
"
text[i] = 0;" chops it off. Then, "text += i + 1;" points over to what remains; it must be remeasured.No need to remeasure, just do a
len = strlen(text);once outside the loop and thenlen -= (i + 1);inside the loop.But now that I take a better look at the code, it contains a bug: whenever a line is 'chopped' with
text[i] = '\0';, we loose a character of the original string.So I'll have to reopen this bug.
In my opinion, that is unnecessary extra code for something that runs seldom and doesn't need to be fast.
The same is true for the character loss. It is a consequence of "in-place" editting. It happens rarely. We already know what the character is. It isn't vital that it be displayed. In my opinion, it isn't worth the extra code that would be needed to show that character.
Dropping data from a message just to avoid writing extra code is a terrible idea. What's next, not displaying 'x blocks free' because the user can calculate that themselves looking at the directory listing?
"Can calculate" is very different from "already knows"! When I wrote my comment, I was thinking of the issue that inspired the fix: When an error message shows the long path-name of a file that failed to load/save, that name is still very fresh in the user's mind because he just chose it (he even might not notice that a character is missing).
But now, a different case has occurred to me: The log can have messages that describe trouble with finding, loading, or parsing files that were named by resources -- they can have long path-names. I can imagine someone creating error messages that show such descriptions on the screen. In those cases, the user will need to see precisely which file failed.
So, I agree, the "in-place" editting scheme needs to be scapped. It currently is done by a separate function. Then, the entire editted stuff is dumped to the screen. The new scheme should do the folding and printing together, line-by-line. The folding-method will be "extraction" instead of "editting". The original mesage won't be alterred; therefore, nothing will be lost.
When I tested the patch, I didn't even notice the missing chars in the error message, I just noticed 'some-long-filename-which-wasnt-even-a-snapshot'. filling in the blanks myself.
But indeed, when it comes to error messages, each character is important. (Though I have no idea how a user would copy/paste from an SDL surface to a text clipboard.)
So yes, the text needs to get 'cut up' properly, allocating enough memory for the extra '\0' 'cut-marks' as it were. Now the question becomes, will you give it a go, or shall I? :)
honestly, if you guys want to make it really proper: the backslashes in the path are displayed are completely wrong, some c64 character.
It's not the backslash, it's the underscore.
It's both! The backslash is shown as the british pound sign (lira). The underscore is shown as the back-arrow.
I'm testing my new patch now. (I had not thought about those pesky ASCII characters; but, substitutions will be easy to do -- already, newlines are replaced by spaces.)
sorry, it was more some sort of joke, i don't think it's worth the effort.
but if you are willing to do it, cool. :)
i mean, we can fool the SDL version in other places too. just try to save a snapshot and type * or ? or <> you will get another "assertion failed".
an i remember other things like this i came across... although, some of it might be fixed by now.
I can't get SDL to assert on '*', '?', '<' or '>', it dutifully saves "*.vsf" for example. Of course SDL shouldn't accept those 'dangerous' characters and should properly check the filename for characters that have special meaning. Probably using some function in archdep.c (*nix will accept just about anything except '/', Windows is a lot more picky).
Last edit: compyx 2017-01-16
I remember why I had not thought about the missing ASCII stuff: I don't see it. A long time ago, I fixed the SDL screen-code mapping table, so that I see PetSCII glyphs that resemble those ASCII shapes. It works throughout the menu system and the monitor. (I will send a separate patch for it.) I did need to substitute slashes for backslashes because I didn't change the british pound sign in the table (the business PET machines have a backslash instead).
Alright, nice. I don't know anything about PETs, but I known their keyboards differs a lot from, for example, a C64. I can live with backslashes in Unix pathnames (for display), not ideal, but it'll work.
I'll see what I can do about SDL's filename/path input being too liberal, most likely some sanitize_path() function are something, using some archdep stuff to determine what is allowed and what isn't.
Here is the new patch. All characters appear.
I meant that the PET models have a backslash where we expect to see it. Therefore, I didn't replace it in the mapping table. (That backslash was replaced by Britain's currency symbol when the VIC-20 was designed.)
Applied the patch, tested it against large filenames/paths, works fine. Thanks!
Now I see what you meant with the backslash, that was in PETSCII before the VIC20, yes. I've only ever had C64's, so the british pound symbol was natural for me. Before thatI had a Philips VideoPac G7000 and a TI 99/4A, no I idea if they ever had a backslash.
I just realized that I left a redundant expression in the code. It assigned a pointer. But, the new code that substitutes better characters sets that pointer, as a side-effect. This amendment removes that now-pointless line.
Thanks, I removed the redundant expression.