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

Interactive CLI theme selection, 7 new themes, new project structure, updated README, bugfix #22

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

dmitchellspace
Copy link

Updates:

  • Added 7 themes from https://github.com/skalidindi3/kicad-colors (all but default)
  • Reorganized structure to clean up top-level
  • Added script to handle finding eeschema & pcnew files based on user operating system, automatic backups, find available themes, display them to the user, and take in user selection before calling patch.py
  • Added Makefile for user simplicity
  • Fixed some pathing bugs with patch.py that caused issues on Windows OS
  • Revamped README with a lot more detailed information and instructions

@jaskij
Copy link

jaskij commented Dec 4, 2020

I'd prefer it if you left the manual installations instructions - I was able to use a theme without even cloning the repository.

There are multiple reasons:

  • How many Windows users will have a CLI with make and Python in PATH?
  • Many will want to look through the script before running it (or at least should) and running random scripts found online is a bad habit anyway
  • Additional requirements for little benefit (I've done those merges/patches manually using Notepad++)

@dmitchellspace
Copy link
Author

Hey @jaskij, thanks for taking a look at the PR.

How many Windows users will have a CLI with make and Python in PATH?

You're correct that many Windows users will likely not have Python installed and it included in their PATH variable. I did preface these instructions by stating the assumptions were that a CLI is used with Python installed. I can add a link to some instructions on how to do this for Windows if you think that would be beneficial. I don't think we should be recommending users to manually make backups and use text editors to copy + paste themes into their configuration files. There is a lot of potential for human mistakes doing that - not to mention a lot of manual work that could be wasted if the user decides they don't like it and want to try another theme.

Many will want to look through the script before running it (or at least should) and running random scripts found online is a bad habit anyway.

I absolutely agree that users should be looking through the scripts before running them. I'm not sure how any of my changes discourage doing that. I actually delve quite a bit into detail on the README about how the scripts work and the order that they're called in so the user can more easily follow the flow when they do read through the code.

Additional requirements for little benefit (I've done those merges/patches manually using Notepad++).

The additional requirements don't apply if the user is just going to use a text editor anyway. Also, to reiterate my point above, I don't think that we should be recommending users to manually edit configuration files.

If you haven't done so already, I highly recommend grabbing these changes and giving them a try for yourself. It's incredibly nice to make, select a theme, and then re-open KiCad to check it out. I was able to try out all of the available themes and pick my favourite in < 5 minutes. Maybe I will make and add a gif to the README demonstrating this.

@jaskij
Copy link

jaskij commented Dec 5, 2020

I only skimmed through those changes in README.

At the very least - please live those configuration paths in place. That was all I needed to manually edit the configuration files without doing any additional googling. Adding a disclaimer that editing those files manually is not recommended is also an option.

People come from many different schools of thoughts and have varying preferences. I for one prefer manually editing configuration files over using wizards and automated tools.

I would probably take a better look at your changes, but currently I do not have Linux install at home.

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