[WIP] refactor, bug fix, optimization

[Rudxain]

• removed some unused vars (IDK if there are more)
• performance improvements
• avoids race condition
• raise clearly understandable Exception if DE is unrecognized
• etc...

What I mean by "race condition", is because of os.getenv("XDG_CURRENT_DESKTOP"). Each call to os.getenv makes a system call, which is slow by itself, and prevents auto-optimization by CPython. Each system call has enough latency, that if a process mutates XDG_CURRENT_DESKTOP at the right time, it can prevent all conditional branches in the if ... elifs from being executed

[Rudxain]

I'll convert to draft, because I forgot to fix the color-scheme for non-green themes

[mtwebster]

Cinnamon was deliberately using a dark theme with a light Gtk theme, but these issues are rendered more or less moot by linuxmint/cinnamon@791ee48, where you'll now be able to quickly choose a light/dark/mixed theme.

It looks like a commit related to that Cinnamon work is blocking this from being merged - if you want to rebase (and leave the Cinnamon theme as is in change_color(), I'll merge the other refactoring/cleanup work.

Thanks

[Rudxain]

I realized there's some more refactoring to do. Please don't merge (yet)

I don't have much time right now, but I'll probably finish it this week (or the next)

[mtwebster]

I think it's admirable the effort you're putting in here and don't want to be discouraging, but this is completely overboard.

I was ok merging what you had done minus the theme changes, but I don't want the thing rewritten, sorry. It's a simple program that works fine for its intended use, and has the potential to change greatly from one Mint release to the next. All this is doing is increasing the potential for new regressions. I think it's actually making the code more difficult to read as well.

We have a lot of repos and I wouldn't mind seeing more contributions and bugfixes, but before attempting something like a major rewrite, I would suggest opening a discussion about it first.

[Rudxain]

@mtwebster , ok, so, should I open a discussion now? Which changes were "too much"? I could revert them

[mtwebster]

You originally were cleaning up repeated calls to get the desktop environment, and other cleanup. That's fine - if you want to pare it down to those again, we can reopen this.

• we don't need typing. There's not enough going on here for that to matter and I'm not sure it's compatible in LMDE5 (python 3.9).
• renaming imports - subp_call vs subprocess.call etc..., apt_Cache.. I've never seen this done to such a degree, and I think it harms readability - there's no need to shorten stuff like this.
• moving strings into constants, enums.. there's not enough code for this to help at all, they're better off at their point of use. When you're working with 1000's of lines of code and re-using strings a lot, it starts to make more sense to do this.

I'm not saying I'm against optimizations, but for nearly all of our gui programs (mintwelcome, mintsources, etc..) readability counts far more than efficiency or making use of python's latest features.

We also have to be careful to maintain compatibility with our debian-based distro - LMDE, which I mentioned only has python 3.9. This is pretty much always going to be the case, as we don't control the parent distros' releases and package versions, so we keep mostly to vanilla code as far as python goes.

Last, smaller PRs are better. They're easier to test - here, you changed so much, I dreaded having to review and test it (and I'd have to do it on all three desktops). Your initial PR was easy to review, as it was focused. It's easy to get carried away (I'm as guilty as anyone). You're also a new contributor and I'm much more careful and critical than with people who've been contributing for months and years.

As for discussion - I guess that's not the right thing to say, you definitely don't need to 'ask' to work on something - doing what's enjoyable for you is the most important thing.

[Rudxain]

if you want to pare it down to those again, we can reopen this.

I agree 👍

• we don't need typing. [...] I'm not sure it's compatible in LMDE5 (python 3.9).

oh... I forgot about that. I'm sorry

• renaming imports [...] I think it harms readability - there's no need to shorten stuff like this.

Ok, I'll keep the names 1-to-1 (. -> _)

• moving strings into constants, enums..

Yep, I think the enums were unnecessary.

there's not enough code for this to help at all, they're better off at their point of use.

For most vars, yes. But stuff like DEFAULT_COLOR is very useful if we want to replace "green" with "aqua"/"blue" in the future. Imagine using find&replace, or worse, manual writing, to replace just 1 color.

It also helps with semantics. Instead of being a hardcoded "green", it's whatever DEFAULT_COLOR contains.

When you're working with 1000's of lines of code and re-using strings a lot, it starts to make more sense to do this.

That's what I did with the var I mentioned. But I agree that other vars (such as MMC) should be inlined.

I'm not saying I'm against optimizations, but for nearly all of our gui programs (mintwelcome, mintsources, etc..) readability counts far more than efficiency or making use of python's latest features.

ok, I'll try to maximize readability

smaller PRs are better. They're easier to test

I agree and "resonate" with this, lol. I understand

here, you changed so much, I dreaded having to review and test it (and I'd have to do it on all three desktops).

I'm sorry (again)

You're also a new contributor and I'm much more careful and critical than with people who've been contributing for months and years.

That's reasonable

As for discussion - I guess that's not the right thing to say, you definitely don't need to 'ask' to work on something - doing what's enjoyable for you is the most important thing.

ok, thanks

[mtwebster]

Like I said, I do not want imports renamed - how is os_get_env an improvement over os.getenv?

[Rudxain]

I did that for 2 reasons:

• Importing a specific module, rather than a collection of modules is (usually) good practice
• The prefix is useful to remember the "parent module" (I'm not sure if this is the correct terminology)

Should I import the parent module (original), or remove the prefix (no as rename)?

[mtwebster]

Importing a specific module, rather than a collection of modules is (usually) good practice

This is meant to discourage using something like from foo import *, so you don't up with unexpected global namespace collisions. It's not intended to be used the way you're using it.

There's no reason to use 'os_get_env' over 'os.getenv', or anything in the os module, for that matter. I'll do it if I'm using just a single function from ridiculously_long_module_name, or if I'm repeatedly using math.pi, it looks cleaner to maybe from math import pi as PI.

You should just import modules and use them normally for the most part.

A bit of info:
https://google.github.io/styleguide/pyguide.html#22-imports
https://peps.python.org/pep-0008/#imports

[Rudxain]

This is meant to discourage using something like from foo import *, so you don't up with unexpected global namespace collisions. It's not intended to be used the way you're using it.

There's no reason to use 'os_get_env' over 'os.getenv', or anything in the os module, for that matter. I'll do it if I'm using just a single function from ridiculously_long_module_name, or if I'm repeatedly using math.pi, it looks cleaner to maybe from math import pi as PI.

You should just import modules and use them normally for the most part.

Fair enough 👍. Since readability is more important than performance, and the Py runtime is very likely to optimize it, and importing 1 parent module pollutes less the global namespace than multiple modules, then it's safe to conclude that it's better not to use my "prefix approach".

A bit of info: https://google.github.io/styleguide/pyguide.html#22-imports https://peps.python.org/pep-0008/#imports

Thanks for the links! I'll read them later. I'm busy IRL

[mtwebster]

Same - we're using these methods once, why rename them?

[mtwebster]

Same...

[mtwebster]

Same

[Rudxain]

Closing as stale.

My temporal bandwidth is very thin now. And I'm no longer a Mint user since many months ago. So I no longer have personal motivation to contribute to these projects. I'm sorry for letting the community down :(

I hope other people continue contributing to these projects, since LM is a decent OS, NGL