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.

Sign up for GitHub

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

Jump to bottom

add: contextMenu items for Run.Plugin.VSCodeWorkspaces #36517

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

add: contextMenu items for Run.Plugin.VSCodeWorkspaces #36517

wants to merge 6 commits into from

Conversation

programming-with-ia
Copy link

@programming-with-ia programming-with-ia commented Dec 22, 2024 •
edited
Loading

Summary of the Pull Request

This pull request addresses and resolves Issue #35773 by introducing the ContextMenuResult functionality to the Run.Plugin.VSCodeWorkspaces plugin.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Fix ✅Issue #35773

Validation Steps Performed

All good from my perspective ☺️.

htcfreek
htcfreek reviewed Dec 22, 2024
View reviewed changes
src/modules/launcher/Plugins/Community.PowerToys.Run.Plugin.VSCodeWorkspaces/Main.cs Outdated
Comment on lines 238 to 239
ShowErrorMessage("Can't copy to clipboard");
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add logging using Wox.Plugin.Logger.Log.Exception().

(Same on the other places.)

This comment was marked as outdated.

Sign in to view

@programming-with-ia

This comment was marked as resolved.

Sign in to view
@programming-with-ia
logs using Wox.Plugin.Logger.Log
80b0068 
fix errors
@programming-with-ia
Add System.Windows.Input for key handling
f4b8150
@programming-with-ia
...
fa03d59 
manually edit Resources.Designer.cs
fix: trailing comma in multi-line initializers
bump plugin version
@programming-with-ia
Copy link
Author

@htcfreek

I need help

@htcfreek
Copy link
Collaborator

@htcfreek

I need help

@programming-with-ia
I need more information about what your problem is.

Btw. The Resources.Designer.cs is auto generated.

htcfreek
htcfreek suggested changes Dec 24, 2024
View reviewed changes
Copy link
Collaborator

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

Please fix the mentioned things. Other than that looks good to me.

I can a a second look after you did the changes.

src/modules/launcher/Plugins/Community.PowerToys.Run.Plugin.VSCodeWorkspaces/Main.cs Outdated
};
}

private ContextMenuResult CreateContextMenuResult(string title, string glyph, Key acceleratorKey, ModifierKeys acceleratorModifiers, Func<bool> action)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please go without this duplicated code. It is except of one ore two properties the same as above.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't quite understand.

Are you suggesting removing the CreateContextMenuResult() method and directly adding the entries into the List<ContextMenuResult>() instead? Similar to this example?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. Otherwise we have duplicated code that acts only as transport code. This is not really needed.

Alternatively you can move all the context menu code into a method and only call the method from the property.

src/modules/launcher/Plugins/Community.PowerToys.Run.Plugin.VSCodeWorkspaces/Main.cs Outdated
}
catch (Exception ex)
{
LogError("Can't copy to clipboard", ex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add the message box too. I think direct user feedback makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

image

src/modules/launcher/Plugins/Community.PowerToys.Run.Plugin.VSCodeWorkspaces/Main.cs
{
var name = $"Plugin: {_context.CurrentPluginMetadata.Name}";
var msg = "Can't Open this file";
_context.API.ShowMsg(name, msg, string.Empty);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why removing existing functionality? Please add message box again.

Copy link
Author

Choose a reason for hiding this comment

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

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically this works. But logging and showing all message are different things. Either you rename the method or you split the methods.

htcfreek
htcfreek suggested changes Dec 24, 2024
View reviewed changes
src/modules/launcher/Plugins/Community.PowerToys.Run.Plugin.VSCodeWorkspaces/Main.cs Outdated
return true;
}

public void LogError(string msg, Exception exception = null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why public. Private should be work. Or not?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ping: @programming-with-ia

Copy link
Author

Choose a reason for hiding this comment

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

@htcfreek
review

Copy link
Collaborator

Choose a reason for hiding this comment

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

@programming-with-ia
The new HandleError() method si still public. And I think private should works and is the better way to go. Please change this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What method would you change else? This PR only implements the HandleError() and this should be done in the correct way.

Copy link
Author

@programming-with-ia programming-with-ia Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @htcfreek,

Looks like there's a build error 😒. Could you guide me on how to rerun this job?
image

Copy link
Collaborator

@htcfreek htcfreek Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately I can't rerun the pipeline. But you can merge current main in. This should trigger the pipeline again.

@programming-with-ia
feat: show error messages
602735c 
remove unnecessary empty lines

`private HandleError()` method
@programming-with-ia programming-with-ia marked this pull request as ready for review December 26, 2024 06:33
@htcfreek htcfreek added the Needs-Review This Pull Request awaits the review of a maintainer. label Dec 26, 2024
@htcfreek
Copy link
Collaborator

@programming-with-ia
I have added the review label because this PR needs a final review from the core team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htcfreek htcfreek htcfreek approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
5. Needs Attention
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@programming-with-ia @htcfreek