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

Enable 'Clear Console' key binding for all consoles (fixes #53) #51

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Enable 'Clear Console' key binding for all consoles (fixes #53) #51

merged 1 commit into from
Jun 30, 2023

Conversation

sratz
Copy link
Member

@sratz sratz commented Jul 8, 2022 •
edited

Previously, in the 'Keys' preference page, binding a key to the
'Clear Console' command, would only affect process consoles, such as
the 'Run as -> Java Application' console.

It did not work in any other consoles.

Solution:

  • Move the 'org.eclipse.debug.ui.commands.console.clear from
    org.eclipse.debug.ui to org.eclipse.ui.console. The ID is kept stable
    to not break any existing key bindings.

  • Set action defintion ID for the clear command to also show an active
    key binding in the context menu.

  • Enable the 'clear console' handler also in in the generic
    TextConsolePage.

Fixes: eclipse-platform/eclipse.platform#541

@sratz sratz requested a review from vogella July 8, 2022 10:27
@vogella
Copy link
Contributor

vogella commented Jul 13, 2022

Currently on holidays, but change looks good to me, please merge if you like.

@SarikaSinha
Copy link
Member

@sratz
Please create an issue as this is not a simple typo kind of change.
@vogella
Have you verified the fix? Just to understand whether I need to verify the fix or not as I could not understand clearly from your comment.

@sratz sratz changed the title Enable 'Clear Console' key binding for all consoles Enable 'Clear Console' key binding for all consoles (fixes #53) Jul 14, 2022
@BeckerWdf
Copy link
Contributor

I tested this.

  1. For "Run as Application" I can now see the key binding in the context menu and the key binding is working as before
  2. For "Run as Eclipse Application" I also can see the key binding in the context menu and the key binding is correctly working. Without this change this did not work in the "Run as Eclipse Application" use-case.
    So from my side this looks good.

BeckerWdf
BeckerWdf approved these changes Aug 17, 2022
View reviewed changes
Copy link
Contributor

@BeckerWdf BeckerWdf left a comment

Choose a reason for hiding this comment

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

See my comments about my manual test

@BeckerWdf
Copy link
Contributor

Have you verified the fix? Just to understand whether I need to verify the fix or not as I could not understand clearly from your comment.

I did verify it.

@mickaelistria mickaelistria added this to the 4.26 M1 milestone Aug 22, 2022
@akurtakov
Copy link
Member

What is the status here? At the verfy minimum manual rebase is needed.

@sratz
Copy link
Member Author

sratz commented Feb 2, 2023

This needs a bit more cleanup work:

Currently, this has a cyclic dependency due to the categoryId="org.eclipse.debug.ui.category.run" being defined in the debug plugin.

The console-related shortcuts should probably get their own category (defined in org.eclipse.console.ui).

I'll clean that up but for post-M2 this is probably more than just a simple bugfix, so I suppose we will re-target this to 2023-06.

@iloveeclipse iloveeclipse removed this from the 4.26 M1 milestone Mar 7, 2023
@sratz
Enable 'Clear Console' key binding for all consoles
4723c6b 
Previously, in the 'Keys' preference page, binding a key to the
'Clear Console' command, would only affect process consoles, such as
the 'Run as -> Java Application' console.

It did not work in any other consoles.

Solution:
* Move the 'org.eclipse.debug.ui.commands.console.clear from
  org.eclipse.debug.ui to org.eclipse.ui.console.
  The ID is kept stable to not break any existing key bindings.
  The Category is changed to org.eclipse.ui.category.edit.

* Set action defintion ID for the clear command to also show an active
  key binding in the context menu.

* Enable the 'clear console' handler also in in the generic
  TextConsolePage.
@sratz sratz added this to the 4.29 M1 milestone Jun 28, 2023
@sratz
Copy link
Member Author

sratz commented Jun 28, 2023

Finally got around to follow up on this.

Instead of creating a new 'Console' category with only a single command in it, I have used the 'Edit' category instead. This fits well with the other related commands such as 'Copy' or 'Select All' used in the console and belonging to that category as well.

When at some point we add new commands to the console, we can then re-organize them into their own category.

@sratz sratz merged commit a5e667d into eclipse-platform:master Jun 30, 2023
3 checks passed
@sratz sratz deleted the clear-command branch June 30, 2023 08:58
@iloveeclipse
Copy link
Member

Note: even if this PR seem to be merged, it was merged after the base repository content was moved to https://github.com/eclipse-platform/eclipse.platform via eclipse-platform/eclipse.platform#524.

Unfortunately the source was not moved right after move, which allowed this PR to be merged without issues.
So please recreate this PR for https://github.com/eclipse-platform/eclipse.platform again.

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

@vogella vogella vogella approved these changes

@BeckerWdf BeckerWdf BeckerWdf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.29 M1
Development

Successfully merging this pull request may close these issues.

Keyboard shortcut assigned to 'Clear Console' command not effective in all consoles
7 participants
@sratz @vogella @SarikaSinha @BeckerWdf @akurtakov @iloveeclipse @mickaelistria