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

Sample implementation of embedding theme files #43

Merged
merged 5 commits into from
Sep 20, 2020

Conversation

LordFlashmeow
Copy link
Contributor

Building on #25, here's a sample of how we can embed the theme and filetypes files into the executable.

@LordFlashmeow LordFlashmeow marked this pull request as ready for review September 18, 2020 23:55
@sharkdp
Copy link
Owner

sharkdp commented Sep 20, 2020

Thank you very much for your contribution!

To be honest, I'd like to avoid doing this. As far as I can see, the only reason would be the packaging difficulties on macOS. I can't really believe that we can not solve this in another way. It should be possible to install a few files to a specified location and load them from there.

@LordFlashmeow
Copy link
Contributor Author

It's not just packaging difficulties with macOS, but packaging difficulties on every platform. Currently, unless you are using debian or Arch, you need to download the repo and copy the files by hand. As I've discovered, every system is different, and the less we depend on the system, the better.

Cargo doesn't let us copy files to a directory when doing cargo install, we need a full-on package manager to handle that for us instead.

If we embed the files, then it is possible for the average user to download the binary from the releases page and it'll work no matter where they put it. Users can also use cargo install vivid without any further action.

The only time a user would need to add a file to the config directory would be if they wanted to customize one of the themes, and even then they also have the option of specifying the exact file via the command line or an environment variable.

Embedding allows:

  • Portable executable
  • No setup for majority of users
  • Minimal change in binary size (less than .1 MB larger)
  • cargo update adds new themes when pushed to repo

@sharkdp
Copy link
Owner

sharkdp commented Sep 20, 2020

You have some excellent arguments, thank you. I will give this a review.

@sharkdp
Copy link
Owner

sharkdp commented Sep 20, 2020

Rust Embed
Rust Custom Derive Macro which loads files into the rust binary at compile time during release and loads the file from the fs during dev.

Oh, that's pretty cool!

@@ -47,9 +52,16 @@ fn load_filetypes_database(matches: &ArgMatches, user_config_path: &PathBuf) ->

let database_path = database_path_from_arg
.or(database_path_env)
.or_else(|| util::get_first_existing_path(&[&database_path_user, database_path_system]))
.ok_or(VividError::CouldNotFindDatabase)?;
Copy link
Owner

Choose a reason for hiding this comment

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

Can we remove VividError::CouldNotFindDatabase in that case?

warning: variant is never constructed: `CouldNotFindDatabase`
  --> src/error.rs:15:5
   |
15 |     CouldNotFindDatabase,
   |     ^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, although we will need to keep the CouldNotFindTheme error

@sharkdp
Copy link
Owner

sharkdp commented Sep 20, 2020

The code changes look great - thank you very much!

@LordFlashmeow
Copy link
Contributor Author

LordFlashmeow commented Sep 20, 2020

    let theme = sub_matches
        .value_of("theme")
        .or_else(|| theme_from_env.as_ref().map(String::as_str))
        .unwrap_or("molokai");

In this bit, it looks like we're setting the theme to molokai when unwrapping the environment theme fails there is no environment theme specified. Can we raise an error in that case?

@sharkdp
Copy link
Owner

sharkdp commented Sep 20, 2020

In this bit, it looks like we're setting the theme to molokai when unwrapping the environment theme fails. Can we just raise an error in that case?

Yes - sounds good. Thanks

@LordFlashmeow
Copy link
Contributor Author

Turns out it's checking if no theme is specified. Should that raise an error?

@sharkdp
Copy link
Owner

sharkdp commented Sep 20, 2020

Well, for now, vivid generate will use molokai as a default if VIVID_THEME is not set. I'm okay with changing this.

Ideally, we would cover all cases:

  • VIVID_THEME unset => vivid generate should throw an error.
  • unknown VIVID_THEME => vivid generate should throw an error.
  • vivid generate <theme> for unknown <theme> => should throw an error.

Since the database is now embedded in the executable, we don't need the CouldNotFindDatabase error. 

Added the NoThemeProvided error when the user doesn't add a theme to generate/preview
@sharkdp sharkdp merged commit d1dd20c into sharkdp:master Sep 20, 2020
@sharkdp
Copy link
Owner

sharkdp commented Sep 20, 2020

Thank you for the update!

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.

None yet

2 participants