Refactored codebase to Python #63
Conversation
|
Nice, and good job! |
|
NICE 👍 |
There was a problem hiding this comment.
Overall, pretty good. Just some things like using global variables, you should instead use an object instance -- put all state related stuff inside a class.
Formatting is also an issue. Even though Python does accept tabs as the indentation level, everyone uses 4 spaces.
I would recommend you to run flake8 on this and fix the issue. It will give you errors when you are not following standard practice.
rsubtil
force-pushed
the
feature/move-to-python
branch
from
17e23f9 to
3ba101b
Compare
|
I will be busy in the next upcoming weeks, so I've marked this as a "draft" until I implement all the changes pending. |
|
good job, man |
rsubtil
force-pushed
the
feature/move-to-python
branch
2 times, most recently
from
b018291 to
f401eb0
Compare
|
Finally had some time to come back to this. I fixed all the other details pointed out, so this should be ready to merge again. Regarding installation, I've decided to stick with meson. I feel it's more important to stick to GNOME packaging guidelines than those of Python, since this is a highly "desktop-integrated" application. Coupled with the fact I have zero experience on Python packaging, and also having no time to learn it properly, I leave it for somebody else to implement this if it's really important and desired 🙂 |
rsubtil
force-pushed
the
feature/move-to-python
branch
from
f401eb0 to
ee33db4
Compare
.flake8
Outdated
| [flake8] | ||
| ignore = | ||
| W503, # line break before binary operator (follow PEP8 style guide) | ||
| E402, # module level import not at top of file (because gi requires to query for versions before importing) |
There was a problem hiding this comment.
I would add # noqa: E402 to those lines instead.
There was a problem hiding this comment.
In this particular case it's not pleasant to add to all the affected lines, since I can't move these imports to above the gi.require_versions(...):
gi.require_versions(...)
from gi.repository import Gio, Gtk, GtkClutter, Gdk, Gst, Clutter # noqa: E402
from komorebi.preferences_window import PreferencesWindow # noqa: E402
from komorebi.settings import ConfigKeys, Settings # noqa: E402
import komorebi.utilities # noqa: E402
from komorebi.screen import Screen # noqa: E402
...But I found a per-file-ignores rule which makes this simpler.
komorebi.py
Outdated
|
|
||
|
|
||
| def check_desktop_compatible(): | ||
| return not('wayland' in os.environ.get('XDG_SESSION_TYPE', '') or os.environ.get('WAYLAND_DISPLAY', '')) |
There was a problem hiding this comment.
This seems weird, are you sure you don't want to do os.environ.get('XDG_SESSION_TYPE') == 'wayland'?
>>> 'wayland' in 'lalalawaylandlalala'
TrueAlso, the default argument fo os.environ.get can be omitted, because it will fallback to None.
>>> os.environ.get('does-not-exist')
NoneThere was a problem hiding this comment.
Yes, this was one of the first files I started with, so my Python knowledge was still very bad 😛
rsubtil
force-pushed
the
feature/move-to-python
branch
from
ee33db4 to
be51be1
Compare
|
Which parts of the codebase depend on Xorg? If i have time and know which parts of the code i have to look at i may implement Wayland support. |
|
@terrarier2111 technically, nothing 😅. You can comment/remove the check preventing Wayland environments to test yourself. I tested on multiple occasions and everything works out of the box, without issues whatsoever. However, some people have had issues with it. The main one I recall is the bubble menu not working; the right-click doesn't get interpreted by Komorebi, getting handled by Gnome Shell instead. If this is really the case, it's a big issue. But, in my environment, I can't replicate it. It might be because I don't use many features of Komorebi too (I just have a simple wallpaper video without anything else, not even the clock) If you can test it and confirm everything works well, it would be reassuring 🙂. In any case, there isn't anything in the code that's Xorg exclusive, so there's really not any pointers I can give. If you find issues however, I can point you to possible places to look at. |
|
Okay, that's great :) I will test if it works as soon as this pr gets merged, so glad that it may work and thanks for your work man, i really appreciate it. |
|
One question don't we have to update the required libs in the README's installing instructions? Because for example we need python3 now and we don't need vala anymore. |
|
I decided to test this pr already, the installation worked but when I tried to start Komorebi, i received this error: https://pastebin.com/ugRXs46K |
Thanks for pointing this out. Apparently when Also just noticed I forgot to save the |
rsubtil
force-pushed
the
feature/move-to-python
branch
from
be51be1 to
9b392f4
Compare
Regarding this, the plan is to do a PR right after this one to bump the version and update instructions, since a release can only be done after this PR is merged. |
|
New error: https://pastebin.com/8wtxLBn8 |
|
Oops, my bad 😅 Alright, should be fixed now. |
rsubtil
force-pushed
the
feature/move-to-python
branch
from
9b392f4 to
9234cb0
Compare
|
I got another one @ev1lbl0w https://pastebin.com/PxT42VNM |
|
That problem looks similar to an open issue, though I can't find it now. @Kangie do you remember which one it was? Something like "can't find the default-application-theme for Adwaita theme", if I remember correctly. Either way, could you open a new issue for it with more info about your setup, please? Also, does it "crash" komorebi, or does it still run despite the errors? |
|
I opened a new issue @ev1lbl0w |
|
So, I somehow missed this. Check it out when I have time! |
|
Any updates? Been a while! |
|
@lmlask I've been busy in the meantime, but yeah I'd still like this to be merged to continue contributing to komorebi. Just waiting for another review to push. |
komorebi.py
Outdated
| screen_list = [] | ||
| for i in range(monitor_count): | ||
| screen_list.append(Screen(i)) |
There was a problem hiding this comment.
List comprehension is faster when appending like that than a regular for loop since the compiler generates bytecode specialization for that call.
| screen_list = [] | |
| for i in range(monitor_count): | |
| screen_list.append(Screen(i)) | |
| screen_list = [Screen(i) for i in range(monitor_count)] |
It also looks prettier :)
rsubtil
force-pushed
the
feature/move-to-python
branch
from
9234cb0 to
b30e8f3
Compare
|
Just a minor concern: is it a good idea to port to python? |
So sorry for the very late reply. Changing the codebase to Python (or any language for that matter) should have little to no perceptible impact, as most of the times komorebi will be running native GTK/Clutter code. And from my daily usage so far, I never noticed any hitch or hiccup when compared to the original code. If komorebi is being resource intensive on video wallpapers, it's likely because hardware accelerated video decoding isn't working properly. Check here for some guides on how to check and fix that. |
|
Hi, any updates on this? |
See #88. Regarding this PR, well, at the time it was working, and I did use the project daily for a few months. Right now I'm not using it anymore, so I cannot guarantee this still works, but if you're interested in contributing, and would like to work under this PR work, let me know and I can merge this. |
|
Let's get this merged at least - I can't see any reason not to and there's a far wider python contributor-base. |
Well, that took a while to complete, but here it is; all Komorebi code, fully ported to Python! 🎉
This is quite the change to review, so take all the time needed to check things! And don't hesitate to ask me questions about design decisions, or what a certain piece of code is doing, or anything really. Below I'll write a checkup of the biggest changes, key decisions, and workarounds implemented:
Refactored
As discussed before, since the move to Python was already a big refactor in it itself, I took the opportunity to also refactor the code itself to be more manageable. The structure is the one detailed below, and thankfully there were no big hurdles that modified the proposed structure, so it should be pretty much what I planned:
Main - Main class that's initialized and stores the global list of Screen's.
Screen - A GtkClutter window that spans the width of an individual screen. It features a single object of type Wallpaper, and a BubbleMenu instance. It can also have multiple Overlay's.
Wallpaper - A ClutterActor that works as the background wallpaper. Every screen uses one, but this is the base abstract type, not instantiated directly; the magic is in it's children.
Overlay - A ClutterActor that is displayed above a Wallpaper. This can be used to implement "meta" elements, independent of the current wallpaper:
BubbleMenu - A menu that allows the user to change wallpaper and global settings, by launching a PreferencesWindow. It's outside of the Overlay class because it's mandatory in every Screen.
Utilities - An utilities file with global functions, variables, and wallpaper/overlay loading.
Settings - The file responsible for holding, managing and storing configurations
PreferencesWindow - A GtkWindow that allows to setup and modify Komorebi's configs and wallpapers. It should interface with Utilities and in some cases Main as well.
The Wallpaper Creator app was also rewritten to Python, but not modified; I didn't bother to refactor anything, as I'm sure this tool will be rewritten to be a much more feature-full editor, so better to write from scratch.
I also had to rename some folders, and move some files around, for easier integration with meson. The most notable is that
srcwas renamed tokomorebi,wallpaper-creatorwas moved to inside of this new folder, and the main files are in the root directory now (komorebi.inandkomorebi-wallpaper-creator.in). This is debatable, so let me know if another structure would be better.Features
NORMAL(normal messages and warnings),INFO(more info messages of what's happening), andDEBUG(verbose description of almost everything). Here's a snippet of the output:More consistent arguments, and all available ones are now explained upon a pretty
komorebi --help.Implementing new wallpapers and overlays is surprisingly plug-and-play; wallpapers simply define their behaviours, connect to needed signals, and that's it. Same for overlays. This opens the door to implement other types of wallpapers and overlays with relative ease.
A workaround would be needed to implement toggling video wallpapers only after a restart... So I cleaned this up and it no longer needs a full reset to work.
Workarounds
Even though the base structure of the code was totally revamped, the wallpaper configuration files weren't; I've decided not to go forward with that right now, as these changes are big as it is, and I don't intend to make breaking changes right away. As a result, there is some code for "workaround" on loading wallpaper configuration; this will be properly fixed when the file structure is rewritten (which I believe is inevitable, if we are to support more complex wallpapers), but for now, I left it as it is.
ClutterGst(the component allowing video wallpapers) is weird; as soon as I set it a player, there's no way to release it. Because of the OOP structure of the code, deleting any fields (which happens when a wallpaper is switched) would cause internal crashes inside Clutter due toNone/nullvalues. Because of it, I had to keep those variables global. It's not pretty, but it's becoming more and more clear that Clutter is a mess (and GTK4 has released already, hooray!), so hopefully when things are moved to GTK this won't be an issue anymore.I faced a problem where the wallpapers and overlays wouldn't be deleted when changing wallpapers, still functioning (and wasting memory) in the background. I eventually found it's because they connect to signals on parent objects (most notably
ScreenandBubblemenu), and because of those dangling connections, Python couldn't delete them. I changed some methods, such as aconnect_weakonScreenandBubbleMenu, for controlling these signals, and also taking care to manually disconnect every signal I did. I fixed the problem for the wallpapers and overlays, which were the biggest issue, but haven't still figured it out for theDesktop. However, since the memory leak caused by this Overlay is pretty small (I toggled it around 50 times and it only increased by 1MB), it's not very urgent right now. I'll see if I can fix it some time later.On another topic, I haven't decided what version bump we should do. Do we perform a patch or a minor bump? I've tried to minimize breaking changes, and I believe this doesn't introduce any. However, this is quite the lengthy code to review, and I've had no shortage of bugs to fix during the rewrite. I expect more to crop up that I haven't caught, so in a sense, this could be a breaking change. I don't know, what do you think guys?