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

Symlink to symlink to file preload support #128

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lch361
Copy link

@lch361 lch361 commented Jan 19, 2024

Hi. I stumbled upon this pretty nice software, but immediately encountered a problem: hyprpaper can preload only a path to file or path to a symlink that gets dereferenced once. Symlinks to symlinks do not work - well, not anymore :)

@lch361 lch361 changed the title Recursive symlink preload support Symlink to symlink to file preload support Jan 19, 2024
@lch361 lch361 marked this pull request as draft January 19, 2024 04:56
@lch361
Copy link
Author

lch361 commented Jan 19, 2024

There is one problem though: recursive symlinks cause an exception and I have no idea how it's better to handle it in this program. Should it become a parse error, or a debug log, or exception is fine?

@lch361 lch361 marked this pull request as ready for review January 19, 2024 05:11
@lch361 lch361 marked this pull request as draft January 19, 2024 05:11
@vaxerski
Copy link
Member

Should it become a parse error, or a debug log, or exception is fine?

probably debug log

@lch361
Copy link
Author

lch361 commented Jan 20, 2024

..so I viewed code a little bit more - and turns out, every call to std::filesystem::exists can throw an exception if too many levels of symlinks. Handling these errors around the project would require a separate pull request, I think.

For now, I'll leave this pull request as is, sorry. I can't go right here through multiple parts of the project to handle every std::filesystem function, there has to be a better way.

@lch361 lch361 marked this pull request as ready for review January 20, 2024 08:28
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

can't we use canonical?

@vaxerski
Copy link
Member

sure, we probably can just write a wrapper func in the future

@lch361
Copy link
Author

lch361 commented Jan 21, 2024

can't we use canonical?

That works too, but also checks whether a file exists (throws an exception/returns an error if it doesn't). And g_pConfigManager->m_dRequestedPreloads contains only paths to existing files. Seems like unnecessary double checking: wallpaper target creation (line 138) should fail anyway if there are any problems with file.

@vaxerski
Copy link
Member

yes but canonical is 1 line meanwhile your thing is like 10

@lch361
Copy link
Author

lch361 commented Jan 21, 2024

Hm.. Then, it may be better to remove exist check in src/config/ConfigManager.cpp (line 70), replace it with std::filesystem::canonical, and leave src/Hyprpaper.cpp without any excessive checks. Or vice versa: have all checks only in src/Hyprpaper.cpp.

@vaxerski
Copy link
Member

exist check won't crash, and it shouldn't. We can leave it intact.

@lch361
Copy link
Author

lch361 commented Jan 22, 2024

It indeed shouldn't crash: instead of throwing an exception, canonical can return an error code through 2nd argument.
In src/Hyprpaper.cpp, we can do something like this:

        std::error_code ec;
        auto create_wp = std::filesystem::canonical(wp, ec);
        if (ec.value() != 0) {
            Debug::log(ERR, "Error preloading %s: %s", wp.c_str(), ec.message().c_str());
            break;
        }

But it takes near-same amount of lines

@vaxerski
Copy link
Member

I still prefer not rewriting STL functions we already included

@lch361
Copy link
Author

lch361 commented Jan 22, 2024

I still prefer not rewriting STL functions we already included

My last written solution is not rewriting any STL functions, as we can see. What's the problem?

@vaxerski
Copy link
Member

canonical does the same in my eyes. It won't throw because of a non-existent file because we check for that.

@lch361
Copy link
Author

lch361 commented Jan 22, 2024

But there is a (albeit very small) possibility that a file gets removed between config parsing and wallpaper target creation. I think best way is to check for file existance only once - that is, only when actually preloading the image. Filesystem is volatile, and changes independently of our program, so sequent checks mean nothing.

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