Modify

Opened 6 years ago

Closed 6 years ago

#15 closed defect (fixed)

'color' command doesn't accept

Reported by: hawke@… Owned by: oxygene
Priority: minor Milestone:
Component: FILO Version:
Keywords: Cc:
Dependencies: Patch Status: patch has been committed

Description

using symbolic color names with the 'color' command (as described in 'help color') doesn't work. It just prints 'Error 23: Error while parsing number'.

Attachments (2)

filo-colors.diff (701 bytes) - added by oxygene 6 years ago.
filo-colors2.diff (3.5 KB) - added by hawke@… 6 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 6 years ago by stepan

What color names did you try?
This seems to work fine here.

Changed 6 years ago by oxygene

comment:2 Changed 6 years ago by oxygene

  • Owner changed from somebody to oxygene
  • Patch Status changed from there is no patch to patch needs review
  • Status changed from new to assigned

what is not supposed to work (according to the current code) is using colors 8..15 as background color, ie. any of these: "dark-gray", "light-red", "light-green", "yellow", "light-blue", "light-magenta", "light-cyan", "white"

The attached patch removes the "blink-"-feature (as it doesn't seem to work anyway), and allows all colors for both foreground and background. Tested on QEmu.

comment:3 Changed 6 years ago by oxygene

  • Patch Status changed from patch needs review to patch has been committed
  • Resolution set to fixed
  • Status changed from assigned to closed

comment:4 Changed 6 years ago by hawke@…

Ah, I had tried with 'red/white'. while that is an invalid background color according to current code (and in help), it shouldn't print 'Error while parsing number'

In fact, it should probably never print that, since there's no requirement that the command have a number.

Changed 6 years ago by hawke@…

comment:5 Changed 6 years ago by hawke@…

Looks like QEmu doesn't work like a real VGA display then.

This was a bad patch. The background color in traditional VGA has no 'bright' bit, and therefore only 8 colors.

So the problem is definitely in the error message, rather than in how the color strings are interpreted.

It turns out the error message comes in when the fallback from color_number() tries to do a safe_parse_maxint() on the string. It assumes that if it's not a valid color specifier string, then it must be an integer rather than simply an invalid color specifier. Naturally it should try to parse it as an integer as well, but if it's not an integer either it should handle that properly.

On another, related note:

The bits in the color byte *as used in filo* are:
7: foreground bright
6: foreground blue
5: foreground green
4: foreground red
3: foreground blink
2: background blue
1: background green
0: background red

This is in contrast with pretty much every VGA color setting method out there:

7: 1 = blinking on; 0 = blinking off
6: Background red component on/off
5: Background green component on/off
4: Background blue component on/off
3: High-intensity foreground on/off
2: Foreground red component on/off
1: Foreground green component on/off
0: Foreground blue component on/off

I've attached a patch which:

  • Reverts the previous patch
  • Makes the FILO color byte match the VGA color byte
  • Makes the 'BG' part optional (black is assumed if it's not specified)
  • Makes the color command use ERR_BAD_ARGUMENT instead of the ERR_NUMBER_PARSING from safe_parse_maxint()

comment:6 Changed 6 years ago by oxygene

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:7 Changed 6 years ago by oxygene

I commited hawke's patches (Changesets 74, 75). Please re-test and close.

comment:8 Changed 6 years ago by oxygene

  • Resolution set to fixed
  • Status changed from reopened to closed

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.