Skip to content

Commit

Permalink
Switched toolbox IDs to signed. (#76)
Browse files Browse the repository at this point in the history 
* Switched toolbox IDs to back to signed
  • Loading branch information
@c-jo
c-jo authored Nov 27, 2023
1 parent f6374d0 commit 130384f
Show file tree
Hide file tree
Showing 21 changed files with 204 additions and 176 deletions.
13 changes: 6 additions & 7 deletions riscos_toolbox/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

def task_name():
try:
name_len = swi.swi('Toolbox_GetSysInfo', 'I0;..I', 0)
name_len = swi.swi('Toolbox_GetSysInfo', 'I0;..i', 0)
name_block = swi.block((name_len + 3) // 4)
swi.swi('Toolbox_GetSysInfo', "Ib", 0, name_block)
return name_block.nullstring()
Expand Down Expand Up @@ -81,8 +81,8 @@ def _handler_block(handlers, add=[]):
[Toolbox.ObjectAutoCreated, Toolbox.ObjectDeleted])

wimp_ver, task_handle, sprite_area = \
swi.swi('Toolbox_Initialise', '0IbbsbI;III',
560, wimp_messages, toolbox_events,
swi.swi('Toolbox_Initialise', 'IIbbsbI;III',
0, 560, wimp_messages, toolbox_events,
appdir, _msgtrans_block, ctypes.addressof(_id_block))


Expand All @@ -103,8 +103,7 @@ def run(application):
if events.null_polls():
flags = flags & ~Wimp.Poll.NullMask
reason, sender = swi.swi(
'Wimp_Poll', 'II;I.I', flags,
ctypes.addressof(poll_buffer))
'Wimp_Poll', 'II;I.i', flags, ctypes.addressof(poll_buffer))

try:
poll_block = bytes(poll_buffer)
Expand All @@ -114,8 +113,8 @@ def run(application):
if event_code == Toolbox.ObjectAutoCreated:
name = ''.join([chr(c) for c in iter(
lambda i=iter(poll_block[0x10:]): next(i), 0)])
obj_class = swi.swi('Toolbox_GetObjectClass', '0I;I',
_id_block.self.id)
obj_class = swi.swi('Toolbox_GetObjectClass', 'Ii;i',
0, _id_block.self.id)
_objects[_id_block.self.id] = Object.create(
obj_class, name, _id_block.self.id)
continue
Expand Down
6 changes: 3 additions & 3 deletions riscos_toolbox/_types.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@

import ctypes

ObjectID = ctypes.c_uint32
ComponentID = ctypes.c_uint32
ObjectID = ctypes.c_int32
ComponentID = ctypes.c_int32


class ToolboxID(ctypes.Structure):
_fields_ = [("id", ObjectID), ("component", ComponentID)]

def __init__(self):
self.id = 0xffffffff
self.id = -1
self.component = 0

def __repr__(self):
Expand Down
34 changes: 14 additions & 20 deletions riscos_toolbox/base.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,19 @@ def find_objects(template):


def create_object(template, klass=None, args=None):
id = swi.swi('Toolbox_CreateObject', '0s;I', template)
id = swi.swi('Toolbox_CreateObject', 'Is;i', 0, template)
if klass:
if args is None:
args = []
_objects[id] = klass(id, template, *args)
else:
obj_class = swi.swi('Toolbox_GetObjectClass', '0I;I', id)
obj_class = swi.swi('Toolbox_GetObjectClass', 'Ii;i', 0, id)
_objects[id] = Object.create(obj_class, template, id)
return _objects[id]


def delete_object(object, recursive=True):
swi.swi('Toolbox_DeleteObject', 'II', 1 if recursive else 0, object.id)
swi.swi('Toolbox_DeleteObject', 'Ii', 1 if recursive else 0, object.id)
if object.id in _objects:
del _objects[id]

Expand Down Expand Up @@ -77,15 +77,15 @@ def show(self,
else:
parent_obj = parent_comp = 0

swi.swi('Toolbox_ShowObject', 'III0II',
flags, self.id, type, parent_obj, parent_comp)
swi.swi('Toolbox_ShowObject', 'IiIIii',
flags, self.id, type, 0, parent_obj, parent_comp)

def hide(self):
swi.swi('Toolbox_HideObject', '0I', self.id)
swi.swi('Toolbox_HideObject', 'Ii', 0, self.id)

@property
def parent(self):
id, comp_id = swi.swi('Toolbox_GetParent', '0I;II', self.id)
id, comp_id = swi.swi('Toolbox_GetParent', 'Ii;ii', 0, self.id)
if id in _objects:
obj = _objects[id]
else:
Expand All @@ -100,37 +100,31 @@ def parent(self):

return namedtuple('parent', ('object', 'component'))(obj, comp)

def _miscop_get_value(self, op, regs):
return swi.swi("Toolbox_ObjectMiscOp", "III;" + regs, 0, self.id, op)

def _miscop_set_value(self, op, regs, *value):
swi.swi("Toolbox_ObjectMiscOp", "III" + regs, 0, self.id, op, *value)

def _miscop_get_signed(self, op):
"""Use Toolbox_ObjectMiscOp to get a signed integer."""
return swi.swi("Toolbox_ObjectMiscOp", "III;i", 0, self.id, op)
return swi.swi("Toolbox_ObjectMiscOp", "IiI;i", 0, self.id, op)

def _miscop_set_signed(self, op, value):
"""Use Toolbox_ObjectMiscOp to set a signed integer."""
swi.swi("Toolbox_ObjectMiscOp", "IIIi", 0, self.id, op, value)
swi.swi("Toolbox_ObjectMiscOp", "IiIi", 0, self.id, op, value)

def _miscop_get_unsigned(self, op):
"""Use Toolbox_ObjectMiscOp to get an unsigned integer."""
return swi.swi("Toolbox_ObjectMiscOp", "III;I", 0, self.id, op)
return swi.swi("Toolbox_ObjectMiscOp", "IiI;I", 0, self.id, op)

def _miscop_set_unsigned(self, op, value):
"""Use Toolbox_ObjectMiscOp to set an unsigned integer."""
swi.swi("Toolbox_ObjectMiscOp", "IIII", 0, self.id, op, value)
swi.swi("Toolbox_ObjectMiscOp", "IiII", 0, self.id, op, value)

def _miscop_get_string(self, op):
"""Use Toolbox_ObjectMiscOp to get a string. This call will allocate
a suitably-sized buffer, read the string and return it."""
buf_size = swi.swi('Toolbox_ObjectMiscOp', 'III00;....I',
buf_size = swi.swi('Toolbox_ObjectMiscOp', 'IiI00;....i',
0, self.id, op)
buf = swi.block((buf_size + 3) // 4)
swi.swi('Toolbox_ObjectMiscOp', 'IIIbI', 0, self.id, op, buf, buf_size)
swi.swi('Toolbox_ObjectMiscOp', 'IiIbi', 0, self.id, op, buf, buf_size)
return buf.nullstring()

def _miscop_set_string(self, op, value):
"""Use Toolbox_ObjectMiscOp to set a string."""
swi.swi("Toolbox_ObjectMiscOp", "IIIs", 0, self.id, op, value)
swi.swi("Toolbox_ObjectMiscOp", "IiIs", 0, self.id, op, value)
6 changes: 3 additions & 3 deletions riscos_toolbox/events.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ def alignment_flags(self):
class UserMessage(Event, ctypes.Structure):
_fields_ = [
("size", ctypes.c_uint32),
("sender", ctypes.c_uint32),
("sender", ctypes.c_int32),
("my_ref", ctypes.c_uint32),
("your_ref", ctypes.c_uint32),
("code", ctypes.c_uint32),
Expand Down Expand Up @@ -196,15 +196,15 @@ def reply(self, reply, recorded=False, size=None,

def acknowledge(self):
self.your_ref = self.my_ref
return swi.swi('Wimp_SendMessage', 'IIII;..i',
return swi.swi('Wimp_SendMessage', 'IIiI;..i',
Wimp.UserMessageAcknowledge,
ctypes.addressof(self),
self.sender, 0)

def _send(self, reason, target, icon, size, reply_callback):
self.size = size or ctypes.sizeof(self)
self.code = self.__class__.event_id
handle = swi.swi('Wimp_SendMessage', 'IIII;..i',
handle = swi.swi('Wimp_SendMessage', 'IIii;..i',
reason, ctypes.addressof(self),
target or 0, icon or 0)
if reply_callback:
Expand Down
22 changes: 11 additions & 11 deletions riscos_toolbox/gadgets/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,12 @@ def create(gadget_type, window, gadget_id):
@property
def flags(self):
"""Gets the gadgets flags."""
return swi.swi('Toolbox_ObjectMiscOp', '0III;I', self.window.id, 64, self.id)
return swi.swi('Toolbox_ObjectMiscOp', 'IiIi;I', 0, self.window.id, 64, self.id)

@flags.setter
def flags(self, flags):
"""Sets the gadgets flags."""
swi.swi('Toolbox_ObjectMiscOp', '0IIII', self.window.id, 65, self.id, flags)
swi.swi('Toolbox_ObjectMiscOp', 'IiIiI', 0, self.window.id, 65, self.id, flags)

def get_flag(self, flag):
"""Gets one gadget flag."""
Expand All @@ -63,36 +63,36 @@ def faded(self, value):
# Wrappers for Toolbox_ObjectMiscOp for some common cases
def _miscop_set_signed(self, op, value, flags=0):
"""Use Toolbox_ObjectMiscOp to set an unsigned integer."""
swi.swi('Toolbox_ObjectMiscOp', 'IIIIi',
swi.swi('Toolbox_ObjectMiscOp', 'IiIii',
flags, self.window.id, op, self.id, value)

def _miscop_get_signed(self, op, flags=0):
"""Use Toolbox_ObjectMiscOp to get an unsigned integer."""
return swi.swi('Toolbox_ObjectMiscOp', 'IIII;i',
return swi.swi('Toolbox_ObjectMiscOp', 'IiIi;i',
flags, self.window.id, op, self.id)

def _miscop_set_unsigned(self, op, value, flags=0):
"""Use Toolbox_ObjectMiscOp to set an unsigned integer."""
swi.swi('Toolbox_ObjectMiscOp', 'IIIII',
swi.swi('Toolbox_ObjectMiscOp', 'IiIiI',
flags, self.window.id, op, self.id, value)

def _miscop_get_unsigned(self, op, flags=0):
"""Use Toolbox_ObjectMiscOp to get an unsigned integer."""
return swi.swi('Toolbox_ObjectMiscOp', 'IIII;I',
return swi.swi('Toolbox_ObjectMiscOp', 'IiIi;I',
flags, self.window.id, op, self.id)

def _miscop_set_string(self, op, text, flags=0):
"""Use Toolbox_ObjectMiscOp to set a string."""
swi.swi('Toolbox_ObjectMiscOp', 'IIIis',
swi.swi('Toolbox_ObjectMiscOp', 'IiIis',
flags, self.window.id, op, self.id, text)

def _miscop_get_string(self, op, flags=0):
"""Use Toolbox_ObjectMiscOp to get a string. This call will allocate
a suitably-sized buffer, read the string and return it."""
buf_size = swi.swi('Toolbox_ObjectMiscOp', 'IIII00;.....I',
buf_size = swi.swi('Toolbox_ObjectMiscOp', 'IiIi00;.....i',
flags, self.window.id, op, self.id)
buffer = swi.block((buf_size + 3) // 4)
swi.swi('Toolbox_ObjectMiscOp', 'IIIibI',
swi.swi('Toolbox_ObjectMiscOp', 'IiIibi',
flags, self.window.id, op, self.id, buffer, buf_size)
return buffer.nullstring()

Expand All @@ -108,11 +108,11 @@ def _miscop_set_font(self, op, name, width=None, height=None, size=None):
raise ValueError("Font height and width or size must be specified.")

if name:
swi.swi('Toolbox_ObjectMiscOp', 'IIIIsii',
swi.swi('Toolbox_ObjectMiscOp', 'IiIisii',
0, self.window.id, op, self.id,
name, int(width * 16), int(height * 16))
else:
swi.swi('Toolbox_ObjectMiscOp', 'IIII0ii',
swi.swi('Toolbox_ObjectMiscOp', 'IiIi0ii',
0, self.window.id, op, self.id,
int(width * 16), int(height * 16))

Expand Down
8 changes: 4 additions & 4 deletions riscos_toolbox/gadgets/button.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ class Button(Gadget):
# Properties
@property
def icon_flags(self):
return swi.swi('Toolbox_ObjectMiscOp', '0III;I',
self.window.id, Button.GetFlags, self.id)
return swi.swi('Toolbox_ObjectMiscOp', 'IiIi;I',
0, self.window.id, Button.GetFlags, self.id)

@icon_flags.setter
def icon_flags(self, flags):
swi.swi('Toolbox_ObjectMiscOp', '0III0I',
self.window.id, Button.SetFlags, self.id, flags)
swi.swi('Toolbox_ObjectMiscOp', 'IiIiII',
0, self.window.id, Button.SetFlags, self.id, 0, flags)

@property
def value(self):
Expand Down
26 changes: 13 additions & 13 deletions riscos_toolbox/gadgets/numberrange.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def value(self, value):
@property
def bounds(self):
return swi.swi(
'Toolbox_ObjectMiscOp', 'IIII;ii',
'Toolbox_ObjectMiscOp', 'IiIi;ii',
NumberRange.LowerBound | NumberRange.UpperBound,
self.window.id,
NumberRange.GetBounds,
Expand All @@ -64,7 +64,7 @@ def bounds(self):
@property
def lower_bound(self):
return swi.swi(
'Toolbox_ObjectMiscOp', 'IIII;i...',
'Toolbox_ObjectMiscOp', 'IiIi;i...',
NumberRange.LowerBound,
self.window.id,
NumberRange.GetBounds,
Expand All @@ -76,7 +76,7 @@ def lower_bound(self, lower):
if lower == self.upper_bound:
lower -= 1
swi.swi(
'Toolbox_ObjectMiscOp', 'IIIIi000',
'Toolbox_ObjectMiscOp', 'IiIii000',
NumberRange.LowerBound,
self.window.id,
NumberRange.SetBounds,
Expand All @@ -86,7 +86,7 @@ def lower_bound(self, lower):
@property
def upper_bound(self):
return swi.swi(
'Toolbox_ObjectMiscOp', 'IIII;.i..',
'Toolbox_ObjectMiscOp', 'IiIi;.i..',
NumberRange.UpperBound,
self.window.id,
NumberRange.GetBounds,
Expand All @@ -98,7 +98,7 @@ def upper_bound(self, upper):
if upper == self.lower_bound:
upper += 1
swi.swi(
'Toolbox_ObjectMiscOp', 'IIII0i00',
'Toolbox_ObjectMiscOp', 'IiIi0i00',
NumberRange.UpperBound,
self.window.id,
NumberRange.SetBounds,
Expand All @@ -108,7 +108,7 @@ def upper_bound(self, upper):
@property
def step_size(self):
return swi.swi(
'Toolbox_ObjectMiscOp', 'IIII;..i.',
'Toolbox_ObjectMiscOp', 'IiIi;..i.',
NumberRange.StepSize,
self.window.id,
NumberRange.GetBounds,
Expand All @@ -117,7 +117,7 @@ def step_size(self):
@step_size.setter
def step_size(self, step):
swi.swi(
'Toolbox_ObjectMiscOp', 'IIII00i0',
'Toolbox_ObjectMiscOp', 'IiIi00i0',
NumberRange.StepSize,
self.window.id,
NumberRange.SetBounds,
Expand All @@ -127,7 +127,7 @@ def step_size(self, step):
@property
def precision(self):
return swi.swi(
'Toolbox_ObjectMiscOp', 'IIII;...i',
'Toolbox_ObjectMiscOp', 'IiIi;...i',
NumberRange.Precision,
self.window.id,
NumberRange.GetBounds,
Expand All @@ -136,7 +136,7 @@ def precision(self):
@precision.setter
def precision(self, precision):
swi.swi(
'Toolbox_ObjectMiscOp', 'IIII000i',
'Toolbox_ObjectMiscOp', 'IiIi000i',
NumberRange.Precision,
self.window.id,
NumberRange.SetBounds,
Expand All @@ -147,7 +147,7 @@ def precision(self, precision):
@property
def numerical_field(self):
return swi.swi(
'Toolbox_ObjectMiscOp', 'IIII;i...',
'Toolbox_ObjectMiscOp', 'IiIi;i...',
NumberRange.NumericalField,
self.window.id,
NumberRange.GetComponents,
Expand All @@ -156,7 +156,7 @@ def numerical_field(self):
@property
def left_adjuster(self):
return swi.swi(
'Toolbox_ObjectMiscOp', 'IIII;.i..',
'Toolbox_ObjectMiscOp', 'IiIi;.i..',
NumberRange.LeftAdjuster,
self.window.id,
NumberRange.GetComponents,
Expand All @@ -165,7 +165,7 @@ def left_adjuster(self):
@property
def right_adjuster(self):
return swi.swi(
'Toolbox_ObjectMiscOp', 'IIII;..i.',
'Toolbox_ObjectMiscOp', 'IiIi;..i.',
NumberRange.RightAdjuster,
self.window.id,
NumberRange.GetComponents,
Expand All @@ -174,7 +174,7 @@ def right_adjuster(self):
@property
def slider(self):
return swi.swi(
'Toolbox_ObjectMiscOp', 'IIII;...i',
'Toolbox_ObjectMiscOp', 'IiIi;...i',
NumberRange.Slider,
self.window.id,
NumberRange.GetComponents,
Expand Down
Loading

4 comments on commit 130384f

@mephillips-durham
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reasoning for changing the toolbox IDs back to signed values?

I have just been using release 1.10 and I think there the IDs are unsigned, but some of the code was passing them to SWIs using the "i" which is signed, so I was sometimes getting errors about the value being out of range for C long. Fo example, in gadgets/slider.py the colour.setter function had the SWI call with '0iiiii' but changing it to '0IIIii' solved the problem.

I would have expected the IDs to be held as unsigned values as they are essentially meaningless, but perhaps there are special values in some of the calls which are actually negative, so they all need to be signed?

Or maybe it was just easier to use signed values because more of the code was set up to expect that?

Just curious as it may help me understand the design better.

@laurenrad
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak for the maintainer, but there's been some back and forth on it and I believe most of the discussion is in these issues:
#75
#78

To attempt to sum it up, it was historically signed values before I used it, and that changed for 1.10 but I suggested it be changed back on the basis that it shouldn't matter as long as usage is consistent, and it was breaking any pre-existing code using the library (namely mine, admittedly). Additionally, I don't believe everything was fully switched to unsigned leaving a couple inconsistencies anyway and the C veneers use the signed convention. However, there hasn't been a tagged release since the switch back, yet. The latest code in main does change back to signed however (although there may still be parts that have been missed, as per some open issues).

@mephillips-durham
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that makes a lot of sense! Yes, as they are opaque values, consistency is the only thing that matters! I'm not so familiar with the Toolbox, but with the Wimp more generally there are certainly instances of window handles having special values such as -1 and -2 for the different ends of the iconbar, so if window handles need to be signed, making Toolbox handles signed as well will be consistent with that.

@laurenrad
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an interesting point, there is at least one or two Toolbox SWIs that I can think of that can return Wimp handles as opposed to Toolbox IDs depending on if a flag is set, I'm not sure if that has any bearing on this but it could be something to consider.

Please sign in to comment.