Skip to content

Conversation

@kyatsen
Copy link
Contributor

@kyatsen kyatsen commented Jan 6, 2026

image

@kyatsen kyatsen requested a review from s8nik January 6, 2026 07:50
Copy link
Contributor

@s8nik s8nik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job.

honesty, the pr is difficult to review, it would be better to split it next time. for this case it could be split into 3 parts:

  1. project setup
  2. fonts and icons sources
  3. code part

also, i think we should remove a lot of the general codebase + configs and only keep what is needed.

@kyatsen
Copy link
Contributor Author

kyatsen commented Jan 7, 2026

good job.

honesty, the pr is difficult to review, it would be better to split it next time. for this case it could be split into 3 parts:

  1. project setup
  2. fonts and icons sources
  3. code part

also, i think we should remove a lot of the general codebase + configs and only keep what is needed.

I've split it into 4 commits, basically as you wrote
image

I think it's easier to review that way

But in the future, I can split per pull request, then by per commit if you want

I left the general codebase + configs because I want to be able to test different scenarios with different backends, I don't see a reason to remove it now. README as well, has usefull information how to run these different backends.
But in general I think the app should support different setups, not only Wayland, someone could want to run
lightweight setup without Wayland DE via SDL for example

@s8nik
Copy link
Contributor

s8nik commented Jan 7, 2026

I've split it into 4 commits, basically as you wrote
I think it's easier to review that way

while i agree that it’s much easier to review per commit, thought in the end everything gets squashed into one large commit on main (which is not good imho). even reviewing it per commit can still be a bit overwhelming since the commits are quite large, it would be even better to split those commits by smaller parts :D

I left the general codebase + configs because I want to be able to test different scenarios with different backends, I don't see a reason to remove it now. README as well, has usefull information how to run these different backends. But in general I think the app should support different setups, not only Wayland, someone could want to run lightweight setup without Wayland DE via SDL for example

hmm for testing purposes, it’s fine to keep different backends if needed, but in the end we should clean this up (honestly, i don’t think there are test scenarios for all of them). my point is that for a standalone app, it makes sense to support all possible backends. but for a specific app that will run only on a specific device, it adds a lot of noise, and most of the general code becomes effectively dead code. since the app’s code will be very specific (it will read data from the specific module), it doesn’t make sense to make it general imho.

@kyatsen
Copy link
Contributor Author

kyatsen commented Jan 9, 2026

@s8nik yeap, I agree, next time will split by MRs, but this is the only big MR for this app.
Ok, I will drop unneeded code later, for now I don't want to spent time on it, because I don't know which exactly
configs we need, and I don't want to backport it back later, easier cut it later then add back.

Copy link
Contributor

@s8nik s8nik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants