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

Feat: Scan theme directory #700

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

Conversation

dipras
Copy link
Contributor

@dipras dipras commented Jul 6, 2023

As requested in src/shared/lib/themes.ts to improve scaning json file instead import one by one
I make the app auto scan all json in themes folder to be theme option for museeks. I used MuseeksAPI as bridge to deliver scan result to frontend because file system cannot be scanned from frontend

Copy link
Owner

@martpie martpie left a comment

Choose a reason for hiding this comment

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

Thanks for that! That could be a great base for custom themes, as we could imagine scanning a custom folder, like ~/.museeks/themes on startup, and users could put their themes there.

Of course that would mean we should do a couple of runtime validations, to make sure the keys are aligned with the latest theme definition, and we should fallback to _system in case a theme is invalid.

But I think we can discuss it in a later diff :)

package-lock.json Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
src/shared/lib/themes.ts Outdated Show resolved Hide resolved
@martpie
Copy link
Owner

martpie commented Jul 6, 2023

Also, Q&A is failing, I should probably add the list of required VSCode extension required to make things work.

@dipras
Copy link
Contributor Author

dipras commented Jul 7, 2023

Thanks for that! That could be a great base for custom themes, as we could imagine scanning a custom folder, like ~/.museeks/themes on startup, and users could put their themes there.

Of course that would mean we should do a couple of runtime validations, to make sure the keys are aligned with the latest theme definition, and we should fallback to _system in case a theme is invalid.

But I think we can discuss it in a later diff :)

Ah yep, that's great idea we can make an custom theme for user. We can do it later after this PR merged

@martpie
Copy link
Owner

martpie commented Jul 9, 2023

So I just tested it, there are two issues:

  • we do not control the order of the order of the themes anymore, which is a problem:
Screenshot 2023-07-10 at 01 55 09
  • packaged apps crash:
Screenshot 2023-07-10 at 01 53 17

As previously said, I don't think it's an important problem to be solved, but we can keep part of this code! My suggestion would be to keep the old behavior, but use this PR as a base for custom themes: could you do the same, but by scanning ~/.config/museeks/themes? Because it's an absolute path, things should not crash.

You can use this API to get the right path: https://www.electronjs.org/docs/latest/api/app#appgetpathname. appData seems the right location, but it's not easily reachable for Windows/Mac, but don't worry, I'll think about a solution. Once the solution works, we can easily change the path to a better one.

No need to do fancy checks right now, you can just steal one of the theme, change its name, update two variables and test it.

wdyt?

Copy link
Owner

@martpie martpie left a comment

Choose a reason for hiding this comment

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

as-per-my-previous-comment

@dipras
Copy link
Contributor Author

dipras commented Jul 11, 2023

So I just tested it, there are two issues:

  • we do not control the order of the order of the themes anymore, which is a problem:
Screenshot 2023-07-10 at 01 55 09 * packaged apps crash: Screenshot 2023-07-10 at 01 53 17 As previously said, I don't think it's an important problem to be solved, but we can keep part of this code! My suggestion would be to keep the old behavior, but use this PR as a base for custom themes: could you do the same, but by scanning `~/.config/museeks/themes`? Because it's an absolute path, things should not crash.

You can use this API to get the right path: https://www.electronjs.org/docs/latest/api/app#appgetpathname. appData seems the right location, but it's not easily reachable for Windows/Mac, but don't worry, I'll think about a solution. Once the solution works, we can easily change the path to a better one.

No need to do fancy checks right now, you can just steal one of the theme, change its name, update two variables and test it.

wdyt?

Okay, i think it's a good idea. i will try to implemant it

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