Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Cyberpunk with network stats theme #136

Merged

Conversation

amiltonjr
Copy link
Contributor

No description provided.

@mathoudebine
Copy link
Owner

Thanks for submitting this updated theme!
I added some comments that you can find in the review.

Can you also add your new theme to the theme list of these files?

  • .github/workflows/system-monitor-linux.yml
  • .github/workflows/system-monitor-macos.yml
  • .github/workflows/system-monitor-windows.yml

This way it will be tested by the CI 👍
Thanks!

@amiltonjr
Copy link
Contributor Author

Thanks for submitting this updated theme! I added some comments that you can find in the review.

Can you also add your new theme to the theme list of these files?

  • .github/workflows/system-monitor-linux.yml
  • .github/workflows/system-monitor-macos.yml
  • .github/workflows/system-monitor-windows.yml

This way it will be tested by the CI 👍 Thanks!

Did it!

@mathoudebine
Copy link
Owner

Thanks! Did you also see my 2 comments on the theme file?

@amiltonjr
Copy link
Contributor Author

@mathoudebine no, I couldn't find your comments

USED:
SHOW: True
SHOW_UNIT: True
X: 28
Copy link
Owner

Choose a reason for hiding this comment

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

The used RAM text value should be moved slightly to the right, in case the value is more than 10GB:
screencap - Copie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that was because library/stats.py had a fixed length for the string converted. I removed that, but I'm not sure if it will impact the other themes, although I think it shouldn't be fixed anyways...

Y: 330
FONT: jetbrains-mono/JetBrainsMono-Bold.ttf
FONT_SIZE: 23
FONT_COLOR: 255, 255, 255
Copy link
Owner

Choose a reason for hiding this comment

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

I think the donwload & upload texts would look better if they use the generale-mono/GeneraleMonoA.ttf font and the FONT_COLOR: 2, 216, 243 cyan color of the theme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did it, thanks for reporting

@mathoudebine
Copy link
Owner

Sorry @amiltonjr i did not validate the comments! You can see them now

@amiltonjr
Copy link
Contributor Author

@mathoudebine I just updated the PR with your suggestions

@@ -428,7 +428,7 @@ def stats():
if THEME_DATA['STATS']['MEMORY']['VIRTUAL']['USED'].get("SHOW", False):
virtual_used = sensors.Memory.virtual_used()

virtual_used_text = f"{int(virtual_used / 1000000):>5}"
virtual_used_text = f"{int(virtual_used / 1000000)}"
Copy link
Owner

Choose a reason for hiding this comment

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

The padding in the value should not be removed: it is used to avoid "screen persistence" when a short value is written after a longer one.
screencap
With the padding we are sure the longer value is erased completely, otherwise the previous unit is still displayed like in the picture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mathoudebine just commited a fix, please try it again and see if it solves the problem

Copy link
Owner

@mathoudebine mathoudebine Jan 27, 2023

Choose a reason for hiding this comment

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

I just tested your new modification, but there is still an issue with changing alignment of the virtual memory value from right to left.
It breaks other themes that are made for the value aligned to the right, like 3.5inchTheme:
after-3 5inchTheme

I'm sorry but this modification cannot be made in the code for all themes.
I will have to think of a way to specify if the value must be left or right aligned in the theme file.

In the meantime, you can remove this modification from the PR so i can merge it here, and keep it only in your repository if you want!

@mathoudebine mathoudebine merged commit e4c75e6 into mathoudebine:main Feb 2, 2023
@mathoudebine
Copy link
Owner

I've merged your PR to unblock other waiting PRs but i reverted the change on the master branch for the memory value alignment as i explained on the previous comment.
You can keep this modification in your repository if you want, in a future version i will add an alignment option in theme.yaml
Thanks!

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.

2 participants