search

ryu1kn / vscode-partial-diff

Visual Studio Code Extension. Take a diff of 2 parts of text(s)
https://marketplace.visualstudio.com/items?itemName=ryu1kn.partial-diff
MIT License
184 stars 15 forks source link

A few enhancements #5

Closed eamodio closed 7 years ago

eamodio commented 7 years ago

Great extension!

Here are a few enhancements I hope you will find useful.

  1. Makes the "diff" command context-aware -- it will only show up after there has been a selection
  2. Renames the commands to be more inline with vscode's file diff commands
  3. Adds the commands into a PartialDiff category -- so they will show in the command palette categorized
  4. Changes the title of the diff editor to provide more contextual information about the comparison

Let me know what you think.

Thank you, Eric

ryu1kn commented 7 years ago

Thank you so much for taking your time to improve this extension! Here are my comments on the each improvement in your PR.

  1. Makes the "diff" command context-aware -- it will only show up after there has been a selection

setContext command is not public yet (vscode issue here) and I wonder if we can wait for their conclusion. Currently, if we execute the takeDiff command first, it will compare your 2nd text against an empty string and it's not too bad.

  1. Renames the commands to be more inline with vscode's file diff commands

Looks good :)

  1. Adds the commands into a PartialDiff category -- so they will show in the command palette categorized

Great! I didn't know that you can specify category attribute; so was manually writing "PartialDiff: " at the beginning of command titles 😛 I raised an issue on vscode-docs to promote this knowledge.

  1. Changes the title of the diff editor to provide more contextual information about the comparison

Looks good. I didn't do it as I concerned about files whose names are too long but maybe we can truncate them if it becomes a real issue. By the way, can we swap the order of file names? As text 1 comes left and 2 goes right.

eamodio commented 7 years ago

No problem!

Comments below:

setContext command is not public yet (vscode issue here) and I wonder if we can wait for their conclusion. Currently, if we execute the takeDiff command first, it will compare your 2nd text against an empty string and it's not too bad.

I can take it out if you want, but my argument for leaving it in is that while it is a "hidden" api -- its been recommended for use with many extensions and given that it is just a single line it should be extremely easy and low risk should the api change in the future. Also, while I agree it isn't a big deal to not have it -- but this extension isn't something people are likely to use constantly -- so having less items in the context menu is important -- at least in my opinion. Your call, of course.

By the way, can we swap the order of file names? As text 1 comes left and 2 goes right.

What issue are you seeing? I see the first selection on the left (with the filename it came from) and the second selection on the right.

eamodio commented 7 years ago

I've made a couple more changes.

ryu1kn commented 7 years ago

but this extension isn't something people are likely to use constantly -- so having less items in the context menu is important

Thank you for clarifying the pros/cons of using setContext command! Although I'd prefer not to use it at the moment, trying to have less items in the menu considering the frequency of using this extension is a truly valid point.

What issue are you seeing? I see the first selection on the left (with the filename it came from) and the second selection on the right.

Sorry, I thought I had seen "B <--> A" on the diff title where diff has A on the left and B on the right... But that is not actually the case.

ryu1kn commented 7 years ago

Changed the commands to only show up if there is a selection in the editor

Actually, selecting the whole text by executing "mark text" command without highlighting any text is one feature of the extension and I'd like to keep the behaviour 😅 Do you mind reverting it back?

eamodio commented 7 years ago

@ryu1kn Sorry for the delay, been swamped these last couple of days. I've added it back in also tweaked some of the readme and package description -- let me know what you think.

ryu1kn commented 7 years ago

No worries! And thank you for updating the PR including descriptions in README & package.json.

I'm merging your PR now. Thank you again for improving the extension!

eamodio commented 7 years ago

Woohoo! Thank you!