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

I add icons #141

Closed
wants to merge 0 commits into from
Closed

I add icons #141

wants to merge 0 commits into from

Conversation

Tobilike
Copy link
Contributor

@Tobilike Tobilike commented Jan 5, 2024

A picture says more
mit icons

I have added a config option to switch the icons on and off as above. However, a nerd font is required for this to work, otherwise you will only see placeholders. https://github.com/ryanoasis/nerd-fonts
"icon": true,

Reason and / or context

I like it.

How has this been tested ?

With my start.py

Types of changes :

  • Bug fix (non-breaking change which fixes an issue)
  • Typo / style fix (non-breaking change which improves readability)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist :

  • [IF NEEDED] I have updated the README.md file accordingly ;
  • [TO DO] I have updated the test cases (which pass) accordingly ;
  • [IF BREAKING] This pull request targets next Archey version branch ;
  • My changes looks good ;
  • I agree that my code may be modified in the future ;
  • My code follows the code style of this project (PEP8).

@Tobilike
Copy link
Contributor Author

Tobilike commented Jan 5, 2024

I made a mistake with Uptime. I have improved it. Don't really know how to change it here. Is it automatic?

@ingrinder ingrinder self-assigned this Jan 6, 2024
@ingrinder ingrinder added the enhancement ⬆️ Implements a new feature, fixes or improves existing ones label Jan 6, 2024
@ingrinder
Copy link
Collaborator

Hi again @Tobilike!

I'll give this a full review later on. For the time being I have a few thoughts after a skim:

  • Would it be possible to rebase this onto our master, particularly so that the changes from Added the endeavouros logo #140 and your master branch aren't part of this PR?
  • On adding icons: instead of making a configuration check and prepending the icon to the name in each entry, I would suggest it may be better to e.g. pass an icon to output and decide there whether to print them or not. At the least we need to move the check into e.g. __init__ rather than using class attributes (which may cause issues with multiple entries of the same type, particularly Custom).
  • What's the purpose of the 9k+ icons.py file?
  • The readme also needs updating for the new configuration option 😃

Let me know what you think 👋

@Tobilike
Copy link
Contributor Author

Tobilike commented Jan 6, 2024

Hi @ingrinder

* Would it be possible to rebase this onto our master, particularly so that the changes from [Added the endeavouros logo #140](https://github.com/HorlogeSkynet/archey4/pull/140) and your master branch aren't part of this PR?

Sure, I have no problem with that. GitHub is still new to me and I have to figure out how it all works. If I do something differently than I should, please give me a hand so I have a better understanding.
Can also remove my fork again, just don't quite know how else to submit something.

* On adding icons: instead of making a configuration check and prepending the icon to the name in each entry, I would suggest it may be better to e.g. pass an icon to `output` and decide there whether to print them or not. At the least we need to move the check into e.g. `__init__` rather than using class attributes (which may cause issues with multiple entries of the same type, particularly `Custom`).

I'll see what I can do.
I wasn't aware that this could cause problems. It is certainly smarter to call it once instead of calling it x number of times.

* What's the purpose of the 9k+ `icons.py` file?

I wrote everything the file does into the file.
I have not used it yet, but the possibility exists.
I found all the icons I used. It is also best to visit the website https://www.nerdfonts.com/cheat-sheet .

Usage:

import icons

print (icons.icons) # for a list of all icons
print (icons.icons['fa_thumbs_up']) # for printing one icon with the name fa_thumbs_up

output:

{'cod_account': '\ueb99', 'cod_activate_breakpoints': '\uea97', ..., ..., ...,
 'weather_wind_west': '\ue354', 'weather_windy': '\ue31e'}
👍

Greetings

@@ -16,6 +16,7 @@
"suppress_warnings": False,
"entries_color": "",
"honor_ansi_color": True,
"icon": False,
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this configuration entry could be renamed to entries_icon ?

@@ -32,6 +32,7 @@ class Distributions(Enum):
DARWIN = "darwin"
DEBIAN = "debian"
DEVUAN = "devuan"
ENDEAVOUROS = "endeavouros"
Copy link
Owner

Choose a reason for hiding this comment

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

This branch will indeed require a rebase into master.



class CPU(Entry):

Copy link
Owner

Choose a reason for hiding this comment

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

Instead of this in every entries, I propose that a new protected _ICON is added to them and when the configuration enables it, the value is retrieved and prepended to _PRETTY_NAME (in Entry base definition) as you suggested.

Comment on lines 76 to 87

# Check Display-Server-Protokoll

session = os.environ.get('XDG_SESSION_TYPE', '')
if session == "x11":
session = "X11"
elif session == "wayland":
session = "Wayland"

if session != "":
self.value = self.value + " (" + session + ")"

Copy link
Owner

Choose a reason for hiding this comment

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

I guess this feature (?) is a separate concern that maybe should be proposed/discussed in another PR ?

icons.py Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this module if it is not needed in Archey 🙏

start.py Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Same here (to allow smooth merge of your branch).

@Tobilike
Copy link
Contributor Author

Tobilike commented Jan 7, 2024

Now everything should be fine. I have reworked it again.
Also tested tested on a other System. (Ubuntu)

@ingrinder
Copy link
Collaborator

Nice, this is definitely looking like a cleaner solution! 👍

There are a couple of minor things left as far as I can see:

  • We already check for Unicode errors in Output and print a message about changing the unicode option, do we need to mention this option also (will using the icons throw unicode errors?)
  • I noticed the docstrings have moved down below the icons, ideally they should stay right at the top of the class/method.
  • This is a bit nitpicky but I think the # icon/# icon and name comments should go, as _ICON and _PRETTY_NAME seems pretty self-explanatory.
  • Make sure to run Pylint, black and isort over the code, I noticed there is an unused Configuration import in kernel.py, unusued os import in window_manager.py and some changed spacing in lan_ip.py.
  • Finally, as a new feature, we need tests! 😁 I'm up for writing some if you want a hand, otherwise I'll let you do it.

Rebasing might be a bit tricky if you're not used to git yet, but here's the approach I'd recommend if you think you're up to it to solve the conflicts we have now (take a backup of the folder just in case...!):

  1. Add this repo as a remote called upstream: git remote add upstream https://github.com/HorlogeSkynet/archey4.git
  2. Fetch the branches: git fetch upstream
  3. Checkout master from this repo into a new branch called upstream-master: git checkout -b upstream-master upstream/master
  4. Checkout your own branch: git checkout Icons
  5. You could rebase entirely on the command line here, but instead it might be better to do an interactive rebase with git rebase --interactive upstream-master, which gives you a text editor interface to cherry-pick commits.
  6. pick the relevant commits and drop the others. For example, I would drop a16a91b through 38a9c28, the two commits d62c6d4 and 0e244b0 (as these belong in your other PR), and 6691a8a (as it deletes a file in a dropped earlier commit and will fail). I think the rest should be fine as they are as they should squash down fairly cleanly.
  7. You might have to run git rebase --continue after saving and closing this file as some of your commits were empty.
  8. Once done and you are sure the outcome is correct you can then rewrite the branch history on GitHub with git push --force

If you can't manage I'm sure we (by which I mean @HorlogeSkynet, as I can't modify commits before merging PRs 😁) can clean it up when merging for you.

Ciao! 👋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ⬆️ Implements a new feature, fixes or improves existing ones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants