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

Replace space in theme name with - or _ #9

Open
AethanFoot opened this issue Apr 6, 2021 · 1 comment
Open

Replace space in theme name with - or _ #9

AethanFoot opened this issue Apr 6, 2021 · 1 comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@AethanFoot
Copy link
Member

Based on AethanFoot/leftwm-theme-dracula-rounded#1, the folder created for themes with a space in their title will also have a space in it. This causes string splitting with the default up and down scripts as $SCRIPTPATH is not double quoted, and is not ideal in general. I feel replacing the space with an _ when creating the folder would be best practice (also removing capital letters would be nice but not needed).

@mautamu
Copy link
Member

mautamu commented Apr 10, 2021

We might want to follow the AUR package naming scheme :

Package names can contain only alphanumeric characters and any of @, ., _, +, -. Names are not allowed to start with hyphens or dots. All letters should be lowercase.

There are a couple different ways to get there:

  • Renaming. We could just rename all of the packages in leftwm-community-themes and then write a function in leftwm-theme that can accept name changes, such as:
    [[changes]]
    type = "Rename"
    package = "Dracula Rounded"
    result = "dracula-rounded"
    action = "mv"
    The problem we'll likely see with this is people might use older leftwm-theme versions that would not yet support this and duplicate the package into two. I think implementing such a feature for future changes would be helpful, however.
  • Style enforcement. We could instead enforce styles in leftwm-theme new, leftwm-theme update and leftwm-theme upgrade, and just update paths accordingly. We'd likely want to also make sure to re-link the current directory.

We could also do a combination thereof, and I suspect that's the direction we'll ultimately want to go. I will likely be a bit busy until May, so I'll add the "Help Wanted" label for now. I'll make a sample task list for anyone wanting to take on this project though (other steps might be necessary, just pitching what this might look like):

  • Adopt a set style guideline and note this change in README.md
  • Submit a PR to leftwm-community-themes changing all names to match the style requirements
  • Write a function that can be called that will move old directory (probably accept either Path or PathBuf) to a new one that matches the name (see install for how that's done).
  • Change installer to rename the folder to the guidelines. As install just wraps git clone, we should be able to rename, see StackOverflow and git2 docs.
  • Add [[changes]] struct and ChangeType enum to config.rs. Likely similar to:
       enum ChangeType {
         Rename,
         Delete,
      }
       struct changes {
         id: UUID, //could also be an integer? 
         type: ChangeType,
         package: String,
         results: String,
         action: Option<String>
       }
  • Implement a utility function (likely in config.rs) that can retroactively rename necessary themes.toml fields (name and directory).

If we put the [[changes]] struct at the top of known.toml, it may be that older versions of LeftWM will be unable to parse the document, which would prevent weird name bugs. However, I think tables have to be at the end of a struct so this might not be possible. Other solutions welcome!

@mautamu mautamu added help wanted Extra attention is needed good first issue Good for newcomers labels Apr 10, 2021
mautamu added a commit to mautamu/leftwm-community-themes that referenced this issue Sep 10, 2021
With leftwm/leftwm-theme#23 merged, we'll want these configurations to be consistent. An update to themes.toml will be required for most people for LeftWM to track, I believe. 

Not ready to merge, blocked on leftwm/leftwm-theme#9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants