-
-
Notifications
You must be signed in to change notification settings - Fork 651
theme presets added #2278
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
base: main
Are you sure you want to change the base?
theme presets added #2278
Conversation
✅ Deploy Preview for conkyweb canceled.
|
|
maybe need to add gitlib2 to dependencies and i didn know how to do it :( |
|
guys build test done! Maybe somebody say me that i do uncorrect? |
Caellian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a preliminary review.
I'm not 100% sure about adding this feature and would like @brndnmtthws input before accepting this PR. It's a neat idea and I see how people would find it useful and convenient, but it also makes it very easy to distribute malicious configs because people won't check the presets/themes before updating them, just like they don't for AUR lol.
We could author an official repo with reviewed configs, but that's also a lot of work or trust to put into someone. So this would mostly be useful to Linux distribution authors.
CMakeLists.txt
Outdated
| set(conky_libs ${conky_libs} PkgConfig::LIBGIT2) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a pretty detailed guide on the Wiki I wrote quite some time ago that describes how to add dependencies and compile-time options, so read that section.
Lua and Vc are added here because they're in thirdparty directory, but all the dependencies pulled from build system are added to conky_libs variable from cmake/ConkyPlatformChecks.cmake file. Search for BUILD_IMLIB2 in CMake files for a discrete example.
| include(Catch) | ||
| endif() | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go through the diff, either here on github in "Files changed" tab or using git diff and remove these unnecessary changes. This space for instace, blank comments later, etc. make the PR harder to review than it needs to be.
src/conky.cc
Outdated
|
|
||
| #ifdef BUILD_BUILTIN_CONFIG | ||
| #include "defconfig.h" | ||
| #include "themespresetmanager.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you added a guard like BUILD_IMLIB2, the include needs to be guarded with ifdefs, as well as any code that's using that.
src/main.cc
Outdated
| const char * THEME_PRESETS_REPO_CLONING_URL = "https://github.com/Cetttok/testRepoForConkyThemes"; // it is example need to create norm repo | ||
| const char * THEME_PRESETS_REPO_PATH = "/var/lib/conky/themes"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these in main.cc? You're declaring them here, just to pass them to the function in another file. Declare them above that function so they're easy to find and modify and remove the arguments. This won't be called from several places, no need to parametrize the function when constants are fine.
However, these should really be configurable at build time, like HTTPPORT is (see cmake/ConkyBuildOptions.cmake). This would allow distro authors to easily swap them out for their own URL.
main.cc is already very large (much like conky.cc). Avoid placing more stuff in these two files.
src/main.cc
Outdated
| // case 'S': | ||
| // std::cout << "optarg " << optarg << std::endl; | ||
| // std::cout << presets.getThemePath(std::string(optarg)) << std::endl; | ||
| // break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before merge, these comments need to be removed if the code isn't used. Very rarely it makes sense to leave code commented out.
src/themespresetmanager.cpp
Outdated
| return false; | ||
| } | ||
| git_reference * mainBranch = nullptr; | ||
| if (git_reference_lookup(&mainBranch, _repo, "refs/remotes/origin/main")!=0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding "main" ref for repositories feels bad. It should be specified in the same place the repository URI is in the build file (e.g. using THEME_REPO_URL and THEME_REPO_BRANCH).
src/themespresetmanager.cpp
Outdated
| if (git_clone(&_repo, _repoUrl.c_str(), _pathToFolder.c_str(), NULL)!=0){ | ||
| std::cout << "SystemGitRepoSource::error while git clone(init url): "<<git_error_last()->message << std::endl; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bad idea. Cloning a repo with themes can result in HUGE amounts of traffic and freeze conky for a very long time. It's possible for themes to include arbitrary assets (like pictures they draw with cairo).
Git allows pulling only a specific (in this case, the requested one) directory:
git init
git remote add origin GIT_URL
git fetch origin
git checkout origin/BRANCH -- path/to/directoryIt's reasonable to expect the theme repository to contain some top-level theme listing. We already use libxml for some things, so it would be a good choice in this case.
<repository>
<theme name="Theme name" path="manjaro/default">
<include>common/lua/my_util.lua</include> <!-- Not really that important for first version/MVP -->
<author>Author 1</author> <!-- You'd ignore these, just don't error if found -->
<author>Author 2</author>
</theme>
</repository>Another issue is that current code will not initialize submodules. You need to check if theme->path is a submodule and tell git to initialize it first.
src/themespresetmanager.h
Outdated
| #include <map> | ||
| #include <string> | ||
| #include <vector> | ||
| #include <git2.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add newline between standard library and git2 library include.
src/themespresetmanager.h
Outdated
| git_repository * _repo = NULL; | ||
| // /std::string _pathToRepo = ""; | ||
| std::string _repoUrl = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't prefix with _ in C++. All _ prefixed names are reserved for compiler and standard library implementations. See this SO answer.
If you're exposing the repo_url() function, then the variable should be named m_repo_url to avoid naming conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for your review! I try to fix all
src/themespresetmanager.h
Outdated
| AbstractThemesSource(); | ||
| virtual bool loadThemesDb() = 0; // or reload | ||
| virtual std::map<std::string, std::string> getThemesDb() = 0; | ||
| virtual std::vector<std::string> loadPossibleAliasOfThemes() = 0 ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run clang format on your code. It will remove 99% of jank like the space between 0 and ; here.
We should really have a CI that fails if the code isn't formatted using the project .clang-format file.
- xml repo config - git submodules support (i fuck that make) - build optionts add - code style change - not full pull - pulling one dir or file - another small fixes
- libgit2 memory leaks fixed - clang-format on all themepresetsmanager module code - increase verbosity of headers - example xml added - presets repo pulling/cloning log
|
to be honest, I don't understand why nix crashes. |
|
Halo guys! I make most of all that @Caellian told me.
|
|
@Caellian i know that it is opensource (free of all) but will be nice (for me) if you maybe say my Judgment for this |
|
apparently, this pull request will soon become last year's 👍 |
created new module (themepresetmanager) and integrate it to conky in main.c.
implemented:
please view and say me what i must correct
i ready to perform yours requirements
Related to #2142
sorry for my English