Skip to content

Conversation

travkin79
Copy link
Contributor

Hi @ghentschke and @jonahgraham,
I'm experimenting with implementing an action "Move to line" similar to CDT (and hope to add other similar actions, too), but I face a few discouraged access issues, since the implementation requires to access some internal classes / interfaces from CDT.

Do you have any ideas how that should be solved in this case? Make some types in CDT public API or find some other way to implement that? (See my PR's code for more details.)

In my PR draft, I copied a few classes and methods from CDT. That's also something, I'd prefer to avoid, but I don't see how.

@ruspl-afed
Copy link
Member

ruspl-afed commented Sep 3, 2024

Dear @travkin79

From PR code it is not pretty obvious what exactly you need from internal CDT types.

I face a few discouraged access issues

Do you mean you have warnings? If so, I would start from PR variant with warnings so we can see what exactly is needed to be more open from main CDT codebase.

@ghentschke
Copy link
Contributor

@travkin79 can you please add a screenshot of the "Move to line" context menu entry. I am not sure where to find "Move to line" in the old C/C++ Editor.

@ghentschke
Copy link
Contributor

I found it!

@ghentschke
Copy link
Contributor

Do you have any ideas how that should be solved in this case? Make some types in CDT public API or find some other way to implement that? (See my PR's code for more details.)

I don't see how to avoid the discouraged access issues yet. IMO they are okay for now. We have a lot of discouraged access issues with LSP4E as well, since there is still no stable API.

@travkin79
Copy link
Contributor Author

travkin79 commented Sep 4, 2024

Hello @ruspl-afed and @ghentschke,
Yes, I see discouraged access warnings in my IDE, due to access to some classes from internal CDT packages.

If we can live with these, it's ok for me for now. Avoiding them would likely require to make some types from CDT public API. I'm not sure if that's desired.

@ruspl-afed
Copy link
Member

Avoiding them would likely require to make some types from CDT public API. I'm not sure if that's desired.

It depends on concrete type and use case. Since CDT LSP most probably will be a part of CDT repository I don't see a problem. In any case, referring to internal CDT types from CDT LSP is much better than creating copies of them.

@travkin79
Copy link
Contributor Author

travkin79 commented Sep 9, 2024

Since CDT LSP most probably will be a part of CDT repository I don't see a problem.

I didn't expect that. Then I'll go on working on my PR and we'll discuss if the needed types / methods from CDT can be made public or if we can live with the discouraged access warnings, ok?

In any case, referring to internal CDT types from CDT LSP is much better than creating copies of them.

I fully agree.

@travkin79
Copy link
Contributor Author

travkin79 commented Sep 11, 2024

I checked which types and operations I need in CDT LSP to implement actions like "Move to line", "Run to line", and "Resume at line". It seems that I only need some types from org.eclipse.cdt.debug.internal.ui.actions like

  • MoveToLineActionDelegate
  • MoveToLineAdapter
  • RetargettableActionAdapterFactory
  • ActionMessages

If discouraged access is not a big problem, I'd prefer to use all the needed classes / operations from CDT instead of copying them to CDT LSP. It seems, we already have a dependency to org.eclipse.cdt.debug.ui bundle anyway.

I think, it's best to adapt my PR accordingly and see if that works for us.

4.33 I builds are no longer available.
@travkin79 travkin79 changed the title Add action "Move to Line" to debugger context menu Add "Move to Line" and related actions to debugger context menu Sep 12, 2024
@travkin79 travkin79 marked this pull request as ready for review September 12, 2024 10:27
@travkin79
Copy link
Contributor Author

Hi @ruspl-afed and @ghentschke,
I adapted my PR. I could re-use the code from CDT. I only had to copy a few icons. It would be great if you would take a look at my changes.
Thanks and best regards.

Copy link
Contributor

@ghentschke ghentschke left a comment

Choose a reason for hiding this comment

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

LGTM

@ghentschke ghentschke merged commit 502154d into eclipse-cdt:master Sep 13, 2024
3 checks passed
@ghentschke
Copy link
Contributor

@travkin79 Thank you!

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.

4 participants