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

Gogh 2.0 #160

Merged
merged 30 commits into from
Dec 1, 2018
Merged

Gogh 2.0 #160

merged 30 commits into from
Dec 1, 2018

Conversation

phenonymous
Copy link
Contributor

@phenonymous phenonymous commented Nov 30, 2018

Gogh 2.0

This PR started with me wanting this to work for tilix but ended up with a much bigger update. I'll outline what this update contains below.

gogh

TOC

List of changes

General changes

  • Added support for tilix
  • Added support for guake 3
  • Added logo before list of themes
  • Added ability to change url to forked url
  • Added ability to view real theme colors if terminal support truecolor
  • Added a script to view all themes without applying the themes
  • Added a pre-check if a profile already exists, this is to avoid installing multiple profiles of the same theme
  • Added two new themes, google-light and google-dark
  • Added option to apply all themes in one go
  • Added a progressbar to view progress made if users chooses more than 5 themes
  • Removed curlsource from all scripts where applicable. This got replaced by bash -c "$(curl <url>)" instead
  • Removed all source commands and replaced with bash -c <file> for remote files and bash for local files
  • Changed all [ tests to [[
  • Changed all 4 space indentations to 2 space indentations
  • Changed all curl and wget commands to include silent option. This is to avoid cluttering the screen

General theme changes

  • Moved gogh_colors to apply-colors.sh
  • Added export before all variables since all source commands got replaced

gogh dot sh

  • Changed function definition of set_gogh
  • Moved terminal check from apply-colors.sh
  • Changed the way terminal check works by looping until $PPID is no longer a shell.
  • Added a subsection that exports some variables that are relevant for tilix

apply-colors dot sh

This script got an complete overhaul - more info further down

  • Added check to see if the script has all the tools it needs in the beginning of the script
  • Defined functions for code that was repeated multiple times
  • Removed the long terminal if statement and replaced it with case
  • Defined functions to apply theme for various terminals
  • Added a dry run that exists the script before applying any themes if test/print-themes.sh was invoked

Tilix support

This update adds support for tilix terminal. tilix support both profiles and color schemes. A code subsection in gogh.sh checks if current terminal is tilix and asks the user if they want to use color schemes instead of profiles. If they do a temporary directory is made and all color schems gets copied over to that folder. When all themes has been processed all color schemes are copied over to the default tilix color scheme directory $HOME/.config/tilix/schemes and the temporary directory is deleted.
If the user aborts before all themes has been processed, the temporary directory gets deleted and no color schemes are copied.

Overhaul of apply-colors.sh

The reason why I completely changed this script is because I found the old script cumberstone to troubleshoot. Instead of if statements I use a case statement instead and from there calls functions that are applicable for the current terminal.
These functions are

  • apply_cygwin
  • apply_darwin
  • apply_guake
  • apply_elementary
  • apply_gtk

With this change I was able to merge pantheon-terminal and elementary-terminal to one function, apply_elementary. Likewise apply_gtk is a merge of gnome-terminal, mate-terminal and tilix. This makes it much easier to add any gtk-vte based terminal in the future.

Issues

Bug fixes not listed in issues

  • Fixed a bug where option 08 and 09 would throw an "Invalid option"
  • Fixed a bug where gnome-terminal version 3 and above would not work because they changed the terminal name identifier to gnome-terminal- note the hyphen
  • Fixed a bug where elementary-terminal in elementary OS 5.0 Juno would not work because they changed terminal name identifier to io.elementary.t

Verification

This is a non-breaking update. I have merged all the latest commits into this PR so you would be able to auto-merge.

The following table illustrates which terminals and OS's I have verified this to be working on.

Terminal Operating system
Gnome terminal v2 Ubuntu 14.04
Gnome terminal v3 Ubuntu 18.04
Gnome terminal v3 Debian 9.6
iTerm2 macOS High Sierra
iTerm2 macOS Mojave
mate-terminal Ubuntu 18.04
tilix Ubuntu 18.04
Guake 3 Ubuntu 18.04
babun (mintty) Windows 10 1803

The verification tests inluded the following:

Tested both locally (from cloned repo) and remote (using the URL mentioned above)
  • Install themes from gogh.sh
  • Install theme directly from a given themes/theme.sh
  • Print all themes from test/print-themes.sh

I have tried to give an as detailed PR as possible without writing an essay. If you have any questions please do ask.

@notlmn
Copy link

notlmn commented Nov 30, 2018

  • Added two new themes, google-light and google-dark

Can you do this in a separate PR, that would make this PR a little bit easy to review. One step at a time.


  • Removed curlsource from all scripts where applicable. This got replaced by bash -c "$(curl <url>)" instead
  • Removed all source commands and replaced with bash -c <file> for remote files and bash for local files

Wouldn't this cause problems for people who do not use bash as their main shell?


  • Added export before all variables since all source commands got replaced

Make sure that you do not pollute global variable space with all the exported variables, make sure to unset them.

test/test.sh Outdated Show resolved Hide resolved
themes/3024-day.sh Outdated Show resolved Hide resolved
@phenonymous
Copy link
Contributor Author

  • Added two new themes, google-light and google-dark

Can you do this in a separate PR, that would make this PR a little bit easy to review. One step at a time.

I will make a separate PR for this once this PR has been merged so to follow the new theme format.

  • Removed curlsource from all scripts where applicable. This got replaced by bash -c "$(curl <url>)" instead
  • Removed all source commands and replaced with bash -c <file> for remote files and bash for local files

Wouldn't this cause problems for people who do not use bash as their main shell?

It will not. Consider this

  • If you call an executable script without specifying which shell (i.e ./script versa bash script) that does not have any shebang will use the current shell as the interpreter
  • If you call an executable script without specifying which shell (i.e ./script versa bash script) that do have a shebang will use the specified interpreter in the shebang regardless of the shell that called it.

Now if you call a script by specifying which shell to use i.e bash script will use that as the interpreter of the script regardless of what your current shell is if the script does not have any shebang.

Since all script files do use a shebang that specifies bash as the interpreter - all scripts will work as long as the user have bash installed, regardless of what your main shell is

  • Added export before all variables since all source commands got replaced

Make sure that you do not pollute global variable space with all the exported variables, make sure to unset them.

This is a good point. I will fix accordingly

gogh.sh Show resolved Hide resolved
gogh.sh Show resolved Hide resolved
@Mgldvd Mgldvd merged commit d03a597 into Gogh-Co:master Dec 1, 2018
@katerynarieznik
Copy link
Collaborator

@phenonymous His is huge and awesome 😲🤩👏
Looking forward to seeing your google themes 👍

@katerynarieznik
Copy link
Collaborator

also @notlmn thanks for your review 🙇‍♀️

Mgldvd pushed a commit that referenced this pull request Dec 1, 2018
@Mgldvd
Copy link
Collaborator

Mgldvd commented Dec 1, 2018

Hi @phenonymous!
Thanks for this. Another amazing PR.
Whenever I see contributions to this repository I am speechless to see how this repository continues to evolve and for all the work you put into it. .
@notlmn thanks too for your review.

@notlmn
Copy link

notlmn commented Dec 1, 2018

@phenonymous Thanks for the PR, I really like rewrites, they always bring something new.

@Mayccoll @rieznik Glad I could be some help.

@notlmn
Copy link

notlmn commented Dec 1, 2018

And I'd suggest from now on, to squash-commit any PR's. Because there is no point importing all the intermediary commits in a PR into the repo, and the original commits are still available in the PR anyway. And again, that's just what I think, and only a suggestion.

Thanks.

@selfup
Copy link

selfup commented Dec 4, 2018

Wow this is awesome! 🎉

@phenonymous
Copy link
Contributor Author

@notlmn Thanks for your review and for giving me a better understanding of git
@Mayccoll Thanks for this repository, you are my inspiration for starting bashhacking in the first place

z-nexx pushed a commit to z-nexx/Gogh that referenced this pull request Sep 12, 2021
Gogh 2.0 🎉   🎉   🎉   🎉 
Another amazing PR 
thank you very much
z-nexx pushed a commit to z-nexx/Gogh that referenced this pull request Sep 12, 2021
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.

Doesn't working Debian Error, no saved profiles found! error: 5:expected value
5 participants