8000 Add new public actions, move existing actions to internal actionCmd by jperedadnr · Pull Request #63 · gluonhq/rich-text-area · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add new public actions, move existing actions to internal actionCmd #63

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
Merged
merged 3 commits into from
Mar 1, 2022
Merged

Add new public actions, move existing actions to internal actionCmd #63

merged 3 commits into from
Mar 1, 2022

Conversation

jperedadnr
Copy link
Collaborator

Fixes #59

The new Actions are non static, and get the viewmodel when control is added to scenegraph. Existing actions are renamed to ActionCmd, and treated as internal.

@jperedadnr
Copy link
Collaborator Author

@eugener No pressure, but any chance you can have a look at this PR?
I have ongoing changes for other issues that depend on it and it'd be better not to pile them in the same PR.

@tiainen
Copy link
Contributor
tiainen commented Mar 1, 2022

Instead of making an AbstractAction class and adding an implementation for each ActionCmd, we could simplify it by making AbstractAction concrete (renaming it to BasicAction?) and directly passing an instance of Function<Action, ActionCmd> to the constructor.

In ActionFactory, you would for instance create the UndoAction as follows:

undo = new BasicAction(control, action -> ACTION_CMD_FACTORY.undo());

The BasicAction constructor looks like this:

public BasicAction(RichTextArea control, Function<Action, ActionCmd> fnGetActionCmd) {
    this.fnGetActionCmd = fnGetActionCmd;
    ...
}

And then the BasicAction.getActionCmd() method can become this:

private ActionCmd getActionCmd() {
    fnGetActionCmd.apply(this);
}

This would also remove the need for the enum Action.ActionType and the BasicAction.ACTION_MAP variable.

@jperedadnr
Copy link
Collaborator Author

@tiainen that's a nice refactor indeed, it really simplifies it.

tiainen
tiainen reviewed Mar 1, 2022
View reviewed changes
@jperedadnr jperedadnr merged commit 276bb41 into gluonhq:master Mar 1, 2022
@jperedadnr jperedadnr deleted the 59-actions branch March 1, 2022 19:24
jperedadnr pushed a commit to jperedadnr/rich-text-area that referenced this pull request Jul 11, 2023
@johanvos
Fix for gluonhq#63 (gluonhq#65)
239eaa4 
Don't print finest logs for connect and attach
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiainen tiainen tiainen approved these changes

@eugener eugener Awaiting requested review from eugener

@abhinayagarwal abhinayagarwal Awaiting requested review from abhinayagarwal

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add disable property to Actions
2 participants
@jperedadnr @tiainen
0