Boxtron: Use Subclass of Luxtorpeda

Overview

This PR updates the Boxtron ctmod to use a subclass of the Luxtorpeda ctmod, stripping it out almost entirely and having a couple of tweaked values. A new helper method was added to facilitate generating the dependencies warning message, and Luxtorpeda's is_system_compatible method was tweaked a little bit so that it could be called by subclasses. This saves subclasses having to implement this themselves and has as little repeated code as possible.

I chose Luxtorpeda as the primary ctmod as it was the most "generic". It didn't have any dependencies and did a lot of what Boxtron did, but without the dependencies check. In my mind this made it the ideal candidate to be the superclass, it has the fewest moving parts.

Luxtorpeda, Roberta, and Boxtron are all sister projects (with Luxtorpeda iirc being the "overarching" one that can use these two, and others). So from that hierarchy, I think using Luxtorpeda as the base makes sense.

Background

When looking into porting more ctmods over to use the GitHub/GitLab download functions, I thought to start with some "low risk" ctmods, so I was deciding between Boxtron, Roberta, and Luxtorpeda. But when looking at them, I noticed they shared a lot of the same code. The only difference really was that Boxtron and Roberta did dependency checks. There were a couple of other smaller differences, such as the extract directory name being hardcoded, but these were easily fixed by making them variables.

At first I stripped out the Boxtron ctmod, made it subclass the Luxtorpeda ctmod, set its extract_dir_name, and then reimplemented is_system_compatible so that it would check for the Boxtron dependencies instead of simply return True which the Luxtorpeda implementation does.

Then, I decided to clean up the logic a little bit, and generate the string using str.join and a list of dependencies. And of course, then I fell down the rabbit hole and decided to make it a re-usable function that generates the missing dependencies string from the list of dependencies (and a ctmod name to display, and the translation context). You could make a bingo game out of it at this point :smile:

Once this was made generic, I tweaked Luxtorpeda's is_system_compatible function to take some optional arguments, so that subclasses could override these and generate the dependencies message relevant to them. is_system_compatible now takes the ctmod name (purely visual for displaying what ctmod needs these dependencies in the error dialog), the dependencies list (default to empty), and the translation context (defaults to ctmod_luxtorpeda).

is_system_compatible will skip running if the dependencies argument is falsey (i.e. empty), meaning by default it won't run at all for Luxtorpeda, because the optional dependencies parameter is falsey. Boxtron overrides is_system_compatible with no arguments, and instead calls super with its own ctmod name, dependencies list, and tr_context provided.

This might seem excessive, but it means we can re-use this for, say, Roberta, which should be straightforward to re-implement the same way Boxtron is in this PR.

Changes

So the overall changes are:

Luxtorpeda:
- Use a variable name to define the extract dir name, so subclasses can override it
- Tweak is_system_compatible to take some optional parameters, allowing subclasses to use it to generate a relevant missing dependencies message
Boxtron:
- Re-implement entire ctmod to be a subclass of Luxtorpeda ctmod
- Override extract dir name to boxtron
- Set dependencies list
- Re-implement Luxtorpeda ctmod's is_system_compatible method to use ct_name = 'Boxtron', deps = self.deps, tr_context = 'boxtron' so that the relevant missing dependencies dialog can be generated for Boxtron
- One Dependency's display name slightly changed from inotify-tools to inotifywait, as the dependencies list reflects the host_which name. We could make a dictionary with a binary name and display name, if we really want to preserve this though!
Util:
- New create_missing_dependencies_message function which generates a missing dependencies string using a list of dependency names and a translation context

Testing

I tested downloading Luxtorpeda and Boxtron on main and on this branch, and the functionality does not appear to be impacted. host_which still performs exactly as it should

Concerns

My old friend QCoreApplication.instance().translate() is sure to come back and bite me here. I know there is some weirdness around how this works, and it might get grumpy about using tr_context as a variable. In that case, maybe giving it '{TR_CONTEXT}'.format(TR_CONTEXT=tr_context) would make it happy. It works in my tests but I'm not using the i18n stuff, so please bonk me if I broke something here :-)

We also call host_which a few times more than we probably need to. For example we call it in all(host_which(dep) for dep in dependencies), which for Boxtron is 3 calls. Then we call it in '\n'.join with our iterator. It might be possible to store the host_which status for dependencies in a list, and maybe use enumerate in our .join call to check the index of the current iteration over the index of the dependency found list. Maybe something like this (untested):

deps_found = [ host_which(dep) for dep in dependencies ]
if all(deps_found):
    return
msg += '\n'.join(f'{dep_name}: {tr_missing if deps_found[i] else tr_found}' for i, dep_name in enumerate(dependencies))

It might help performance a little if we use this for a ctmod which could have a lot of dependencies maybe. Though there'd probably need to be a lot of dependencies for any noticeable performance impact :-)

Future Work

The changes in this PR could be applied to Roberta as well, and if this approach is accepted, I can implement this as well.

This PR does not move Luxtorpeda over to using the GitHub/GitLab methods mentioned, because that change can be made independent of this work. If this approach is accepted, we can move Roberta over to using this ctmod.

The code in this ctmod is probably sharable with some other """simpler""" ctmods for lack of a better term, such as Steam-Play-None. However, since that project is independent of Luxtorpeda, it would be better to move its repeated code into a util function if possible. Or, probably more useful, it could be moved into a base ctmod class, once we reach that stage :-)

Thanks!

The host_which changes I talked about seem to work well. I also caught a silly logic mistake, where I didn't invert the host_which case properly, so it was returning found for missing dependencies and vice versa. Fixed that and changed the `host_which call to only happen once in abb7893 :-)

Thanks!

Making similar ctmods subclasses of each other, especially when they are related in some way, makes sense.

I chose Luxtorpeda as the primary ctmod as it was the most "generic"

Makes sense when looking at your code and reading your explaination.

then I fell down the rabbit hole and decided to make it a re-usable function

That's good. I wonder, now that we have so many "ctmod utility functions", would it make sense to put them in a separate file, e.g., ctmodutil.py? Not the scope for this PR, just a thought.

Once this was made generic, I tweaked Luxtorpeda's is_system_compatible function to take some optional arguments, so that subclasses could override these and generate the dependencies message relevant to them
super().is_system_compatible(ct_name = 'Boxtron', deps = self.deps, tr_context = 'ctmod_boxtron')

I wonder if there is a "more efficient" way to do this.

Parameter deps: I think we can just do self.deps inside of is_system_compatible. It will be set for the current object in the constructor anyway, no matter if it is a child class or not.
Parameter tr_context: I don't think the translation system will like this. I don't even think it will matter as the text is the same for all ctmods. The dependency list is injected using .format independently from the translations. This is also true for create_missing_dependencies_message. (For context: At runtime, the translation system may be able to handle a dynamic context string, but pyside6-lupdate won't be able to generate correct translation file templates from this).
Parameter ct_name: Not sure about this. CT_NAME is neither a static attribute (like CT_URL), nor an instance variable (like self.deps). We may need to leave it like this, but ct_name = CT_NAME could make it a bit cleaner at least.

One Dependency's display name slightly changed from inotify-tools to inotifywait, as the dependencies list reflects the host_which name. We could make a dictionary with a binary name and display name, if we really want to preserve this though!

That's fine. I guess, it's even better as the user can search by the executable name then. For example, yum whatprovides inotifywait will return inotify-tools-...

My old friend QCoreApplication.instance().translate() is sure to come back and bite me here. I know there is some weirdness around how this works, and it might get grumpy about using tr_context as a variable

Interesting that it works, but I feel like it is not intended this way. See above.

This PR does not move Luxtorpeda over to using the GitHub/GitLab methods mentioned, because that change can be made independent of this work. If this approach is accepted, we can move Roberta over to using this ctmod.

Also moving Roberta over to this ctmod would be great. Yeah, that sounds like something for a separate PR.

Or, probably more useful, it could be moved into a base ctmod class, once we reach that stage :-)

I guess we could deepen the hierarchy.

Maybe like this (nice, Mermaid.live is supported on GitHub :smile:):

graph RL;
A(Boxtron) --> C
B(Roberta) --> C
C(Luxtorpeda) --> E
D(DXVK) --> E
DA(DXVK Async) --> D
E(BaseCtmod)

The host_which changes I talked about seem to work well.

Great

I will do some testing and merge if all is fine

I wonder, now that we have so many "ctmod utility functions", would it make sense to put them in a separate file, e.g., ctmodutil.py? Not the scope for this PR, just a thought.

Yeah, I was also thinking about some kind of way to break out the util functions. Having some kind of core ctmod class is what stopped me from looking into it too deeply but that's probably going to be a gradual change, and having a more broken up set of utils would make sense.

Not to get too deep into it, but maybe we could have a separate util folder, for our steamutil, heroicutil, util, and any future files. Not really a huge detail, but if we go the road of making more util files, I don't think it would hurt much to make a separate folder to keep them organised into.

Parameter deps: I think we can just do self.deps inside of is_system_compatible. It will be set for the current object in the constructor anyway, no matter if it is a child class or not.

Sounds good, would be an easy change and would make sense.

Parameter tr_context: I don't think the translation system will like this. I don't even think it will matter as the text is the same for all ctmods. The dependency list is injected using .format independently from the translations. This is also true for create_missing_dependencies_message. (For context: At runtime, the translation system may be able to handle a dynamic context string, but pyside6-lupdate won't be able to generate correct translation file templates from this).

Ahhh, hmm, I'm not sure how we solve for this... I guess in that case we can't even pass a string to create_missing_dependencies_message using a static variable like CT_TR_CONTEXT? Since the issue is that the dynamic name can't be picked up by the translation dependency pyside6-lupdate.

We could always just generate the string in create_missing_dependencies_message and translate it elsewhere, but I'm not sure how to get around setting the translation string for different ctmods. Maybe there's a way we can give this string a more "generic" context, such as ct_errors?

Also moving Roberta over to this ctmod would be great. Yeah, that sounds like something for a separate PR.

It seems to be virtually identical to Boxtron, just with different dependencies, from what I recall. Should be a straightforward PR :-)

Maybe like this (nice, Mermaid.live is supported on GitHub 😄):

Yup, this is pretty much what I had in mind! Also, very useful markdown tip :smile:

Updated is_system_compatible to use self.deps and removed the deps parameter.

Addressed both feedback points, the error message still shows up correctly with self.tr in the Luxtorpeda ctmod, and with 'util.py' being hardcoded as the translation context in create_missing_dependencies_message. I think this is cleaner overall :-)

I guess the remaining point then, if this resolves the translation issues, is around passing the ctmod name to is_system_compatible. I do see what you mean I think, it's a bit strange to give the ctmod name to a method called by its super class, maybe it would make more sense if the ctmod was aware of its own name.

We could try to solve this by moving CT_NAME to be a static member of CtInstaller, we would need to somehow refactor ctloader.py to account for this change though, since it expects these values to just be module variables I believe? But I wonder if we can do something like ctmod.CtInstaller.CT_NAME. But then we would have to change every ctmod to use this...

The alternative you suggested of just using ct_name = CT_NAME may be the easiest solution, it'll remove the hardcoded string and we can still default to this in LuxtorpedaInstaller#is_system_compatible.

Switched to using is_system_compatible(ct_name = CT_NAME), at least for now :-)

DavidoTek / ProtonUp-Qt