Refactor: Avoid imports inside functions

[MattHag]

Information

• Solaar version: latest master branch

Is your feature request related to a problem? Please describe.
No

Describe the solution you'd like
Imports should always be on top of a module. To prevent circular dependencies, some imports were moved into function calls. Remove this code smell by refactoring the code accordingly.

[pfps]

Yes, a good idea, the question is how.

[MattHag]

I am working on it and achieved some progress, but some imports are really hard to replace.

There are several tricks to achieve it, but the code structure with tight coupling doesn't really help to achieve the goal. However, as tight coupling needs to be removed for unit tests as well, it is a necessity and part of the necessary cleanup.

Simple tricks to avoid circular dependencies are

• splitting modules
• replace module level variables with functions

More advanced

• Modularize functions/class and pass functions/objects as dependencies to avoid tight coupling. May use partial to add coupling at run time.

[pfps]

Oh, I did a bit of work too.

Most of what I did was to look at the major circularity in the logitech_receiver modules. The problem is that there is a central structure - DEVICES in descriptors.py - that is used in low-level code but includes information from setting_templates.py. Splitting the construction of this structure would eliminate the major (and maybe only) circularity there. I have an idea on how to do this. If you are not working on these modules then I can proceed.

Then there is also the use of GUI calls in logitech_receiver.

[MattHag]

Yeah carry on.

[pfps]

PR #2265 breaks up the imports loop with descriptors but there still remains an import loop in logitech_receiver

[pfps]

There is (only) one remaining imports loop inside logitech_receiver.
When the MouseGesture setting in setting_templates.py finishes it starts rule processing by calling process_notification in diversion.py. But diversion.py imports device.py which imports setting_templates.py. I'm trying to figure out how to break this loop. Any hints would be appreciated.

One way would be to have a run-time structure that contains functions that are called in loops. Is this a Pythonic way of doing things?

[pfps]

PR #2265 breaks all the import loops inside logitech_receiver.

There are about 10 calls to GUI functions that still need intra-function imports.

[pfps]

PR #2265 breaks all the import loops inside the rest of the code, except for the calls to GUI functions from logitech_receiver code.

[pfps]

PR #2265 also moves almost all of the other imports to the top of files. The exceptions are conditional imports.

[MattHag]

Looks like you have achieved quite much lately. Most importantly are the ones marked with the circular import comment. It's great when they are gone.

[pfps]

Unfortunately these are the ones that are difficult to get rid of without changing how the code works. The question is what is the best way to proceed. One way would be to have some connection data like a class containing the entry points needed to tickle the GUI. Is that a good way to proceed?

[pfps]

I don't think this will go into release 1.1.11 as it touches a lot of files in non-trivial ways.

[MattHag]

I have never worked with GTK and similar UI, I need to check out the code to suggest something. But Protocols are good for decoupling stuff and with callbacks that could make the components more "self contained".

[pfps]

That's probably the way to decouple the GUI and the rest of the code.

But a lot of code in logitech_receiver is concerned with interpreting information received from devices that support HID++ messages. A good example is hidpp20.py where just about every function and method uses the call feature_request, which gets HID++ information from a device. Testing these functions, in their current state, requires some way to simulate how this function works.

About the only functions that do not use feature_request are associated with the lighting and profiles classes. This code has to build up objects from a chunk of binary data that is stored on the device and that data can be obtained in several ways. So there is code that obtains the binary data from a device and code that uses this binary data to construct useful objects.

But more typical is code like:

def get_polling_rate(device):
    state = feature_request(device, FEATURE.REPORT_RATE, 0x10)
    if state:
        rate = _unpack('!B', state[:1])[0]
        return str(rate) + 'ms'
    else:
        rates = ['8ms', '4ms', '2ms', '1ms', '500us', '250us', '125us']
        state = feature_request(device, FEATURE.EXTENDED_ADJUSTABLE_REPORT_RATE, 0x20)
        if state:
            rate = _unpack('!B', state[:1])[0]
            return rates[rate]

It doesn't seem reasonable to split this into a bit that gets the binary data and a bit that extracts structure from the binary data. It is useful to do a unit test on this that checks that the code sends correct HID++ messages and does the right thing with the result. But to do a unit test here requires simulating how feature_request works, or simulating how something that feature_request calls works, and this requires some sort of HID++ I/O simulation.

A separate issue here is that there needs to be a way to construct the device structure, but that can also be done if there is a way to simulate HID++ I/O.

[MattHag]

I'll check out the piece of code the next days at least. The feature requests can be mocked or passed as callback, need more context to propose an improvement.

[MattHag]

Independently, I suggest adding type hints, as this really helps with code understandability. And it is easy to grasp, what a function returns or expects.

[pfps]

I was looking at how to mock these calls - this seems like a good way to set up the tests.

And, yes, type hints would help a lot. I should have been adding them as I made changes, but a lot of Solaar code dates from before type hints, and I got into the habit of not using them when I made changes.

[pfps]

I started a document on the implementation of Solaar, available at https://pwr-solaar.github.io/Solaar/implementation, partly for my own use. You might want to take a look at it.

[MattHag]

Thanks, that's helpful.

[pfps]

Solaar currently stores information about device using two different mechanisms. Some information is stored in device objects. Some settings store their own inforation. (Some settings, mostly the ones that use complex data, use information stored in device objects.) Worse, changes to data that do not come from the GUI use several mechanisms to update the GUI including direct calls to GUI code that generally require in-function imports to prevent circular imports. This is not a good idea and the storage of device data should be regularized.

There are two differnt ways that I see to go.

The first way is to store all information directly on device objects. The device class is responsible for reading the information from a device, caching the information locally, and writing changed information to the device. Settings then use this data. The device class stores callbacks for each chunk of information, which are invoked whenever the information changes. (Except that maybe calls to change information can provide a callback not to be invoked.) This achieves separation at the cost of requiring setting up separate device information and settings.

The second way is to store all information in settings of device objects. The device class is just responsible for creating these settings. The setting contains methods to read, cache, and write the setting information. Each setting can have callbacks, which are invoked when the setting information is changed. This only requires one class for each chunk of information instead of, potentially, two.

I'm (reluctantly) leaning towards the first way because it allows GUIs to set up their own settings, perhaps just to have an order for settings.

In any case there should be support to reduce the required boilerplate:

Here is how this might work for some simple data, here a boolean value read and written by a feature call that is used only for this purpose

class CrownSmooth(DeviceInformation):
    feature = _F.CROWN
    data_type = bool
    rw_options = {'read_fnid': 0x1, 'write_fnid': 0x2}
    validator_options = {'true': 0x01, 'false': 0x02, 'read_skip': 1, 'write_prefix': b'\x00'}

The information is stored under the name "CrownSmooth". The information type is bool. Data is read using command 1 of feature CROWN and written using command 2. The relevant data is in the second byte of read messages and should be prefixed with a 0x00 byte when writing. The data value for True is 0x1 and for False is 0x2.

class CrownSmooth(Setting):
    label = _('Crown smooth scroll')
    description = _('Set crown smooth scroll')

The setting is presented using the given label and tooltip. It uses the CrownSmooth data of Device and has the datatype of that data.

Here is a slightly more complex setup, using integer values that are further interpreted in the setting.

class BacklightDuration(DeviceInformation):
    feature = _F.BACKLIGHT
    data_type = int

The data is an integer read as the first byte of command 0 of feature BACKLIGHT and written using command 1.

class BacklightDuration(Setting):
    label = _('Backlight')
    description = _('Set illumination time for keyboard.')
    validator_class = _ChoicesV
    choices_universe = _NamedInts(Off=0, Varying=2, VeryShort=5, Short=10, Medium=20, Long=60, VeryLong=180)
    validator_options = {'choices': choices_universe}

There are only a few possible values, given above along with display names for them. The currently valid values are always the same as the possible ones.

Here is a situation where the data is stored as a structure and there are several settings that access the structure.

class Backlight2(DeviceInformation):
    """Information about the current settings of x1982 Backlight2 v3, but also works for previous versions"""
    feature = _F.BACKLIGHT2
    fields = ['mode', 'level', 'dho', 'dhi', 'dpow']

    def read(self, device):
        response = device.feature_request(FEATURE.BACKLIGHT2, 0x00)
        enabled, self.options, supported, effects, self.level, dho, dhi, dpow = _unpack('<BBBHBHHH', response[:12])
        self.auto_supported = supported & 0x08
        self.temp_supported = supported & 0x10
        self.perm_supported = supported & 0x20
	self.mode = (self.options >> 3) & 0x03 if self.enabled else 0xFF
 	levels = device.feature_request(_F.BACKLIGHT2, 0x20)
	self.level_value_range = [ 0, reply[0] - 1]
 	self.dho = dho * 5  # convert from 5 second units to seconds
	self.dhi = dhi * 5
	self.dpow = dpow * 5
	self.dho_value_range = 	self.dhi_value_range = 	self.dpow_value_range = [ 0, ? ]

    def write(self, device):
        self.options = (self.options & 0x07) | (self.mode << 3)
        level = self.level if self.mode == 0x3 else 0
	enabled = 0 if self.mode == 0xFF else 1
	dho = (self.dho + 4) / 5  # use ceiling in 5-second units
	dhi = (self.dhi + 4) / 5  # use ceiling in 5-second units
	dpow = (self.dpow + 4) / 5  # use ceiling in 5-second units
        data_bytes = _pack('<BBBBHHH', enabled, self.options, 0xFF, level, dho, dhi, dpow)
        self.device.feature_request(FEATURE.BACKLIGHT2, 0x10, data_bytes)

class Backlight2(Setting):
    label = _('Backlight')
    description = _('Illumination mode of keyboard.')
    data = 'Backlight2'
    field = 'mode'

    @classmethod
    class validator_class(ChoicesValidator):
        def build(cls, setting_class, device, backlight):
            choices = _NamedInts()
            choices[0xFF] = _('Disabled')
            if backlight.auto_supported:
                choices[0x1] = _('Automatic')
            if backlight.perm_supported:
                choices[0x3] = _('Manual')
            if not (backlight.auto_supported or backlight.temp_supported or backlight.perm_supported):
                choices[0x0] = _('Enabled')
            return cls(choices=choices)

class Backlight2Level(Setting):
    label = _('Backlight Level')
    description = _('Illumination level on keyboard when in Manual mode.')
    data = 'Backlight2'
    data_field = 'level'
    min_version = 3

class Backlight2Duration(Setting):
    data = 'Backlight2'
    min_version = 3
    min_value = 1
    max_value = 120  # actual maximum is 2 hours

class Backlight2DurationHandsOut(Backlight2Duration):
    data_field = 'dho'

class Backlight2DurationHandsIn(Backlight2Duration):
    data_field = 'dhi'

class Backlight2DurationPowered(Backlight2Duration):
    data_field = 'dpow'

[pfps]

PR #2312 adds a callback to signal changes to settings. This removes the remaining imports inside functions, except for one to show an on-screen notification.

@MattHag Any suggestions for removing this last non-top import? The notifications use icons from the GUI, but the icon code and the notify code doesn't depend on anything else in the GUI and could be moved up a level, which probably would allow the imports to be at top level. Alternatively, there could be a callback added for this purpose, freeing the code in logitech_receiver from any dependence on code in the lib/solaar tree. If this is done, what's a good way of setting up and populating the callback?

[MattHag]

I couldn't find the remaining part.

[pfps]

It's in settings_templates around line 778

        from solaar.ui import notify as _notify  # import here to avoid circular import when running `solaar show`,

[MattHag]

A callback passed to the class instantiation should be the easiest for this issue.

However, to get rid of the dependency from the module, the factory build needs to be moved to a new module too. In this case (callback or object etc. from another module is required) the instantiation has to happen in another module, as this instantiation of DivertKeys requires two otherwise independent modules and tightly couples them at instantiation. The basic idea behind this is the factory pattern. In this particular case, the setup is not much code and does not have conditions, but the decoupling is what helps us here.

OT:
The factory pattern might help us to clean up the mostly OS dependent instantiation of modules and hide the details behind a class with common interface. Wherever used, it's also easy to replace modules or lower level APIs.

[pfps]

@MattHag The desired end-state is to disconnect the logitech_receiver code and the Solaar code so that they can be run in different processes. That's a ways off.
But I think that this means that the solution here is for logitech_receiver to have its own notify code.

[pfps]

I think that all these are gone now.

[MattHag]

But I think that this means that the solution here is for logitech_receiver to have its own notify code.

Duplicating a complex code piece should never be necessary. Instantiation aka tight coupling of a class, that relies on several modules should just happen outside all the related modules e.g. in a new file factory.py, which depends on the implementation of notify and logitech_receiver.

[MattHag]

I think that all these are gone now.

Yes, looks quite good. As last step for this ticket I‘d remove all those import as statements with module level imports and no prefixing with underscores.

[pfps]

This has to be done a bit carefully to guard against name clashes.

[MattHag]

Yes. The topic of this ticket is achieved and we can close this ticket. There's still #2273 to handle code cleanup.