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

feat: update rename functionality #141

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

cdaguerre
Copy link
Contributor

@cdaguerre cdaguerre commented Apr 12, 2021

Hi,
To my understanding, the current rename implementation doesn't work anymore.
I'm even wondering if it worked at some point because of the comment that @lexi-lambda made in the PR description: atom/atom-languageclient#270

Anyways, it seemed to rely on the archived atom-ide-ui...

So here's a draft PR to suggest two different implementations:

Both approaches have advantages and drawbacks, eg.

Commands and context menu

  • You can't conditionally disable context menu items asynchronously: the refactor > rename item is always available and will trigger an error when no rename is possible at the given position
  • I found no way to pass the mousevent click position to the command, so the command uses the text editor's buffer position which we set manually on context menu right click.

Intentions

  • Feels less "native"
  • No right click possible, requires a keybinding
  • Rename action is only suggested when possible (using LSP prepare functionality)

@cdaguerre cdaguerre marked this pull request as draft April 13, 2021 13:56
@@ -0,0 +1,37 @@
import { TextEditor } from "atom"

export default class Dialog {
Copy link
Member

Choose a reason for hiding this comment

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

I actually created a dialog view for some other atom packages https://github.com/UziTech/atom-modal-views. Do we want to use that instead of creating our own?

/cc @aminya

Copy link
Member

Choose a reason for hiding this comment

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

I actually created a dialog view for some other atom packages UziTech/atom-modal-views. Do we want to use that instead of creating our own?

That sounds like a good plan! I love reusing code.

@aminya aminya changed the title Update rename functionality feat: update rename functionality Apr 13, 2021
@aminya
Copy link
Member

aminya commented Apr 16, 2021

The last time I checked the intentions package was quite buggy. For example, it disabled selecting the text, or the underlines were longer than what they should be. I thought @steelbrain had stopped supporting it.
steelbrain/intentions#76
steelbrain/intentions#74
steelbrain/intentions#77

So, if we need to add a new package for renaming I am ok with it.

@aminya
Copy link
Member

aminya commented Jun 14, 2021

@aminya aminya marked this pull request as ready for review June 15, 2021 06:14
@aminya aminya added the enhancement New feature or request label Jun 15, 2021
@aminya aminya closed this Jul 22, 2021
@aminya aminya closed this Jul 22, 2021
@aminya aminya reopened this Jul 22, 2021
@ivan94fi
Copy link

Hi, any updates on this functionality? Will this be merged in the near future?

Thank you all from atom-community for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants