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

Bringing custom shellcontextmenu #699

Merged
merged 2 commits into from
Jun 20, 2020

Conversation

BuraChuhadar
Copy link
Contributor

No description provided.

@yaira2
Copy link
Member

yaira2 commented Apr 27, 2020

@BuraChuhadar This is really exciting! Do you want me to mark this as a draft for now?

@BuraChuhadar
Copy link
Contributor Author

Sounds good please mark it as draft. I am still working on it.

@yaira2
Copy link
Member

yaira2 commented Apr 28, 2020

@BuraChuhadar Will this solve #719?

@BuraChuhadar
Copy link
Contributor Author

BuraChuhadar commented May 1, 2020

@yaichenbaum I am currently working on phase 1. So I am not planning to bring File > New options yet. For those the full menu needs to re-written to close #719 . I am not sure how do you want to approach to that.

@yaira2 yaira2 mentioned this pull request May 1, 2020
@yaira2
Copy link
Member

yaira2 commented May 1, 2020

@yaichenbaum I am currently working on phase 1. So I am not planning to bring File > New options yet. For those the full menu needs to re-written to close #719 . I am not sure how do you want to approach to that.

Lets keep this limited to open with for now, however to plan ahead, is there a central place in the registry to pull a list of the item types you can create?

@FelixZacher
Copy link
Contributor

I think i need some clarification on this.

As far as i understand this retrives the "open with" options for a filetype from the registry.
Does this include additional options, than using Windows "ApplicationPicker"? Or what are the benefits of using this approach instead of the "ApplicationPicker".

@BuraChuhadar
Copy link
Contributor Author

You cannot bring File > New options from the registry. You would need to read it from the shell 32 DLL instead.

Copy link
Contributor

@lukeblevins lukeblevins left a comment

Choose a reason for hiding this comment

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

Please address comment regarding WindowsPrincipal

@lukeblevins
Copy link
Contributor

Sounds good please mark it as draft. I am still working on it.

Just saw this now. Let me know if this should be a draft.

Hopefully my feedback helps guide you, but I really like these changes so far,

@BuraChuhadar BuraChuhadar changed the title WIP: Bringing custom shellcontextmenu Bringing custom shellcontextmenu May 4, 2020
@BuraChuhadar
Copy link
Contributor Author

@yaichenbaum I finalized the code no need to draft it. Please take a look. I am seeing WindowsPrincipal class is being supported. If you still think that isn't the case or if there is a way to check if it is supported under UWP then I can take a look.

Copy link
Contributor Author

@BuraChuhadar BuraChuhadar left a comment

Choose a reason for hiding this comment

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

@lukeblevins
Copy link
Contributor

@BuraChuhadar The stack overflow link you sent stated that the API is intended for use inside a brokered component rather than the UWP component.

It doesn't seem supported here: https://docs.microsoft.com/en-us/dotnet/api/?view=dotnet-uwp-10.0&term=principal

@BuraChuhadar
Copy link
Contributor Author

OK, I will take a look no problem to address WIndows Principal code.

@lukeblevins
Copy link
Contributor

@BuraChuhadar Actually, I think it would be better if we removed that part altogether because modern Windows will essentially deprecate the concept of admin vs standard user. With the WinRT sandbox, users expect to be standard users, so we should always assume they are.

@lukeblevins lukeblevins removed their request for review May 17, 2020 01:30
@BuraChuhadar BuraChuhadar force-pushed the develop branch 2 times, most recently from 69011fc to 35e95b5 Compare May 18, 2020 02:06
@BuraChuhadar
Copy link
Contributor Author

BuraChuhadar commented May 18, 2020

@duke7553 it has been a while I couldn't focus on this. I added the changes you mentioned. Let's finish the phase one of this implementation. I will continue adding more features around this. Let me know if it looks good now. The administration security check has been removed from the code.

@BuraChuhadar
Copy link
Contributor Author

BuraChuhadar commented Jun 13, 2020

@yaichenbaum this is ready for review. I took a step back and changed my decision from adding the icons due to restrictions of IconElement class under MenuFlyout class. I would wait to see WinUI 3.0 will deliver something better.

I am going to attach this discussion related to the FlyoutMenu icons: microsoft/microsoft-ui-xaml#1494

I updated the current:

1- Bringing CommandLineToArgvW to parse command registry from Win32API
2- Execution of the context menu was a problem for some applications. This has been fixed.
3- Improved MUIVerb support
4- Changed the ordering of the context menu items. Screenshot:

image

At this point this PR getting quite large. I would suggest let's merge this in as I will continue with other approaches to improve the overall quality with the context menus to pair up the functionality as much as I can with Windows' native explorer.

I am looking forward to hearing about the feedback.

Copy link
Contributor

@lukeblevins lukeblevins left a comment

Choose a reason for hiding this comment

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

Very excellent work! Users will love this feature

@yaira2 yaira2 requested a review from tsvietOK June 14, 2020 02:19
@tsvietOK
Copy link
Contributor

Exception when right clicking on the txt file.
image

@BuraChuhadar
Copy link
Contributor Author

@tsvietOK I will add a null check around that in that case you won't be able to load the extra shell menu items. Thank you for sharing it.

@BuraChuhadar
Copy link
Contributor Author

Never mind. I found the problem. I sent a fix for it and fixed the merge conflicts.

@tsvietOK
Copy link
Contributor

tsvietOK commented Jun 16, 2020

@BuraChuhadar For some reason extended context menu displaying only after second right click on folder(or select it before right click). As i understand for the moment extended context menu is not working for files.

@BuraChuhadar
Copy link
Contributor Author

@tsvietOK it should work for files as well. I didn't try directly right clicking. I will check once I have the chance.

@BuraChuhadar
Copy link
Contributor Author

@tsvietOK sent another fix. I see that the opening of the context menu wasn't synchronizing with the UI correctly given that the registry call method was async. I sent a fix by simply waiting for the registry method read task to complete. Please let me know if you see anything else. Thanks.

@yaira2
Copy link
Member

yaira2 commented Jun 17, 2020

@BuraChuhadar I tested the latest changes and for the first time, I was able to get it working 🎉. It feels kind of off without the icons, we are using custom icons in menus in a couple of places so if you are running into issues, let us know so we can work together to improve the UI.

@tsvietOK
Copy link
Contributor

@BuraChuhadar I am probably doing something wrong, but when i right clicking on file, displaying only standard context menu. Can you share example screenshot on how it should look like.

@BuraChuhadar
Copy link
Contributor Author

BuraChuhadar commented Jun 20, 2020

@yaichenbaum I won't be addressing the icons as I mentioned above. Native explorer has the same behavior. You are welcome to send another PR for the icons.

@tsvietOK please send me your reproducible steps. Otherwise, I would suggest merging these changes and address issues on later tickets. You are welcome to assign to me the upcoming issues.

Here is the video of working changes with directory and files. It is a youtube video click on it to see it under youtube:

@tsvietOK
Copy link
Contributor

@BuraChuhadar maybe i don't have supported apps installed.
Can you check, should Notepad++ be displayed when right clicking on file?

@BuraChuhadar
Copy link
Contributor Author

@tsvietOK this feature won't use shell context extensions. Which in this case Notepad++ is using shell context extensions. I am seeing there are articles around Microsoft not interested in supporting shell context extensions and yet there is still software out there using this. Even PowerToys under Microsoft project is using this.

I will try to bring shell context extension support with a different PR at the moment this PR supports only static shell context menus.

@lukeblevins
Copy link
Contributor

@BuraChuhadar If this is ready to merge, I'd like to do so considering the vast amount of work you put into this. I'm really impressed!

@BuraChuhadar
Copy link
Contributor Author

BuraChuhadar commented Jun 20, 2020

@duke7553 it is ready to merge. Thank you for the feedback.

@lukeblevins lukeblevins merged commit a02295a into files-community:master Jun 20, 2020
@yaira2
Copy link
Member

yaira2 commented Jun 21, 2020

@yaichenbaum I won't be addressing the icons as I mentioned above. Native explorer has the same behavior. You are welcome to send another PR for the icons.

@BuraChuhadar Great work on this PR! As far as I can tell, File Explorer does show the icons in the right click context menu.

@BuraChuhadar
Copy link
Contributor Author

@xpoppyx you won't see all the available items in context menu from the explorer. This feature only implements the static context menus, not the dynamic ones. File a bug and assign it to me if you see there are potential issues with reproducible steps.

@BuraChuhadar
Copy link
Contributor Author

@yaichenbaum not all the items under the native explorer has icons that's what I meant. MenuFlyout also doesn't support bitmap images so I am OK leaving like this. If you have a proposal or a helper method to assign bitmaps to a menu flyout component then I can bring the icons.

@yaira2
Copy link
Member

yaira2 commented Jun 21, 2020

@yaichenbaum not all the items under the native explorer has icons that's what I meant. MenuFlyout also doesn't support bitmap images so I am OK leaving like this. If you have a proposal or a helper method to assign bitmaps to a menu flyout component then I can bring the icons.

That makes sense, in the future we should try to get that working. In the meantime your work helped implement one of the most requested features 🎉.

@BuraChuhadar
Copy link
Contributor Author

@yaichenbaum implementing that would be challenging because the dynamic shell context menus are using COM implementation and depending on the STA or MTA thread apartment the rendering of the shell context menu changes. I am thinking to see if I can bring something by using PowerShell or cmd to query the shell context menu and render it like that. I will create another PR once I see it is possible.

{
AllView.SelectedItems.Add(item);
AllView.SelectedItem = selectedItem;
Copy link
Contributor

@tsvietOK tsvietOK Jun 21, 2020

Choose a reason for hiding this comment

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

What purpose of this change? This broke file selection. File size on status bar is not more visible when changing view mode from GridViewBrowser to GenericFileBrowser and back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants