Hi @StSav012 thanks for the PR! This LGTM.

As a side note, should self.opts be mutable? It doesn't look safe.

Given that it's a dictionary, and python dictionaries are mutable, yeah we should definitely assume it's mutable.

@ixjlyons you going to be able to look at this? if not, this looks ok to merge (would have to look at int casting vs. the use of round.

Hi, @j9ac9k

Given that it's a dictionary, and python dictionaries are mutable, yeah we should definitely assume it's mutable.

I thought of returning a copy of self.opts, i.e. of making it a property. To change the options, there is self.setOpts function, which performs some checks.

I'm not opposed to something like this, but likely care would have to be taken to ensure we don't break stuff for existing users. Also, I'm not sure if this has come up in the issue tracker yet either.

As a side note, should self.opts be mutable? It doesn't look safe.

This pattern is pretty common across the library, so it probably deserves a separate discussion/issue. I'm almost sure there are people relying on opts being mutable where there may be missing setters. Agreed that it's not a great situation though.

the default step is 1 in "int mode"

It's not actually so. I haven't figured out how to make it really a default value. Therefore, I don't understand whether step should be at least 1 in “int mode”.

As for the docstrings, I altered the ones I found.

As a side note, I have a temptation to fix a bizarre code of minStep in this PR, just not to produce a separate PR for every single line change. Check the following code out:

            if 'minStep' in opts:
                step = opts['minStep']
                if int(step) != step:
                    raise Exception('Integer SpinBox must have integer minStep size.')
            else:
                ms = int(self.opts.get('minStep', 1))
                if ms < 1:
                    ms = 1
                self.opts['minStep'] = ms

The code after else can be reduced to the last line: self.opts['minStep'] = 1. However, now for minStep, should we check that in “int mode” it's at least 1?

the default step is 1 in "int mode"

It's not actually so. I haven't figured out how to make it really a default value. Therefore, I don't understand whether step should be at least 1 in “int mode”.

Ah, right. It's minStep that takes over in that case because the default step 0.1 becomes 0. If we really want to set the default to 1, I think the simplest way would be something like this in __init__:

if kwargs.get("int", False):
    self.opts["step"] = 1

It's not that important from a behavior standpoint, but it is a little odd that minStep takes over and is documented as being relevant only in "dec" mode.

As for the docstrings, I altered the ones I found.

As a side note, I have a temptation to fix a bizarre code of minStep in this PR, just not to produce a separate PR for every single line change. Check the following code out:

            if 'minStep' in opts:
                step = opts['minStep']
                if int(step) != step:
                    raise Exception('Integer SpinBox must have integer minStep size.')
            else:
                ms = int(self.opts.get('minStep', 1))
                if ms < 1:
                    ms = 1
                self.opts['minStep'] = ms

The code after else can be reduced to the last line: self.opts['minStep'] = 1. However, now for minStep, should we check that in “int mode” it's at least 1?

The only thing I can think of is dynamically toggling int mode:

>>> sb = pg.SpinBox(int=False, minStep=5)
>>> sb.setOpts(int=True)
>>> sb.opts["minStep"]
5

The proposed change would make it 1 instead. There might be other subtle ways toggling int mode on/off breaks things anyway, so I don't know if it's worth handling it. I'm inclined to leave it though.

Dear @ixjlyons

The code I'm pointing at is fired when 'minStep' is not in self.opts.

            if 'minStep' in opts:
                step = opts['minStep']
                if int(step) != step:
                    raise Exception('Integer SpinBox must have integer minStep size.')
            else:
                ms = int(self.opts.get('minStep', 1))
                if ms < 1:
                    ms = 1
                self.opts['minStep'] = ms

The change I propose now is

            if 'minStep' in opts:
                step = opts['minStep']
                if int(step) != step:
                    raise Exception('Integer SpinBox must have integer minStep size.')
            else:
                self.opts['minStep'] = 1  # the code reduces to this single line

Here's the code that triggers the block after else:

sb = pg.SpinBox(int=False, minStep=5)
print(f"{'minStep' in sb.opts = }")  # True
del sb.opts["minStep"]  # yes, delete
print(f"{'minStep' in sb.opts = }")  # False
sb.setOpts(int=True)  # here, the else part works
print(f'{sb.opts["minStep"] = }')  # 1

In the original code, get certainly returns 1, that is no less than 1, so ms stays equal to 1, and sb.opts["minStep"] becomes therefore 1. No branching. Maybe the code should be outside the first if block:

            if 'minStep' in opts:
                step = opts['minStep']
                if int(step) != step:
                    raise Exception('Integer SpinBox must have integer minStep size.')
            ms = int(self.opts.get('minStep', 1))
            if ms < 1:
                ms = 1
            self.opts['minStep'] = ms

The check that self.opts['minStep'] is no less than 1 makes perfect sense. Is that what has been intended in the piece of code?

Sorry for letting this sit for so long. The changes as they are now look good to me.

@StSav012, I see now I had overlooked self.opts being populated at the top of setOpts before getting to the "sanity checks" section. If you're still interested proposing further cleanup, it might be best to do so in a follow-up pull request.

Thanks for reviewing @ixjlyons and thanks for your submission @StSav012 merging!

Belay that, just saw the CI failures pretty sure they're unrelated but I'm going to verify.

Going to close/reopen this PR to trigger a new CI Run since I can't seem to figure out how to do it on the CI page 😬