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

[WIP] Context menu #372

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

[WIP] Context menu #372

wants to merge 3 commits into from

Conversation

zbalkan
Copy link
Contributor

@zbalkan zbalkan commented May 13, 2022

This is a work in progress.

The PR solves #331. It fulfills the minimum requirements. It is hard to test shell extensions.

@Klocman
Copy link
Owner

Klocman commented May 20, 2022

Let me know when it's ready for review, I'll look into it for the next release.

@Ivanmatthew
Copy link

This is a work in progress.

The PR solves #331. It fulfills the minimum requirements. It is hard to test shell extensions.

Is there a reason why you explicitly get the shortcut target file by using binary file reading?
Optionally it's possible to implement it without calling for a filestream, which sounds more performant in my eyes (correct me if I'm wrong please). e.g. this stackoverflow post

Please think about it if you're still working on this.

@zbalkan
Copy link
Contributor Author

zbalkan commented Mar 6, 2023

Hi @Klocman,

Life happened I couldn't work on this, and actually, I even forgot I worked on this.

When I checked the So page you mentioned, I saw the comment which I used as a reference: https://stackoverflow.com/a/29749787/5910839

It is actually in the comments of GetShortcutTarget method. The intent was to minimize the dependencies but eventually it got out of that direction. In order to simplify, using Shell32 may be easier to implement. But it needs to be tested. I cannot work on it in the short term.

Feel free to close, archive, edit or rewrite the PR.

N.B: Sorry for the late response.

@ghost
Copy link

ghost commented Dec 7, 2023

Pls Update - Has this not yet been implemented?

@zbalkan
Copy link
Contributor Author

zbalkan commented Dec 7, 2023

This was never a part of the road map but an optional thing. I implemented the basics yet it is far from perfect. If you like, you can try to compile and alpha-test my version so that we can fix the issues beforehand.

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.

3 participants