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

Control clicking exercise opens submit window #447

Closed
wants to merge 2 commits into from
Closed

Control clicking exercise opens submit window #447

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee80e66
Control clicking exercise opens submit window
jaakkonakaza Jan 8, 2021
f3cb133
Separate listener sets for different clicks
jaakkonakaza Jan 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions src/main/java/fi/aalto/cs/apluscourses/ui/base/TreeView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;
import java.util.Collections;
import java.util.HashMap;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.swing.SwingUtilities;
Expand All @@ -34,7 +34,7 @@ public class TreeView extends com.intellij.ui.treeStructure.Tree {
}
};

private final transient Set<ActionListener> nodeAppliedListeners = ConcurrentHashMap.newKeySet();
private final transient HashMap<String, ActionListener> nodeAppliedListeners = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be ConcurrentHashMap


@NotNull
public static SelectableNodeViewModel<?> getViewModel(Object node) {
Expand Down Expand Up @@ -98,12 +98,12 @@ private void update() {
restoreExpandedState(expandedState);
}

public void addNodeAppliedListener(ActionListener listener) {
nodeAppliedListeners.add(listener);
public void addNodeAppliedListener(String key, ActionListener listener) {
nodeAppliedListeners.put(key, listener);
Copy link
Member

Choose a reason for hiding this comment

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

In my previous comments, I overlooked one issue. As implemented here, this allows only one listener per key. It's possible that we never need more than one but at least in that case the name of this method should reflect that.

Anyways, I would probably just instead of key-listener map have to separate set of listeners, one per an event type. And add and remove methods for both.

}

public void removeNodeAppliedListener(ActionListener listener) {
nodeAppliedListeners.remove(listener);
public void removeNodeAppliedListener(String key) {
nodeAppliedListeners.remove(key);
}

@NotNull
Expand Down Expand Up @@ -151,9 +151,12 @@ protected class TreeMouseListener extends MouseAdapter {

@Override
public void mouseClicked(@NotNull MouseEvent e) {
Copy link
Member

Choose a reason for hiding this comment

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

do you think the logic inside the method can be tested fully or maybe at least to the extent of the conditional statements?

if (e.getClickCount() == 2 && selectedItem != null) {
if (e.getClickCount() == 1 && e.isControlDown() && selectedItem != null) {
ActionEvent actionEvent = new ActionEvent(this, ActionEvent.ACTION_PERFORMED, null);
nodeAppliedListeners.forEach(listener -> listener.actionPerformed(actionEvent));
nodeAppliedListeners.get("submitExercise").actionPerformed(actionEvent);
} else if (e.getClickCount() == 2 && selectedItem != null) {
ActionEvent actionEvent = new ActionEvent(this, ActionEvent.ACTION_PERFORMED, null);
nodeAppliedListeners.get("openSubmission").actionPerformed(actionEvent);
Copy link
Member

Choose a reason for hiding this comment

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

TreeView should be a domain-ignorant class; it shouldn't deal with concepts such as "submission". It's a general purpose tree widget.

So if anything, the keys should be something like "doubleClick" and "ctrlClick", or maybe more preferably something abstract like "nodePrimaryAction", "nodeSecondaryAction".

I wouldn't, however, introduce any magic strings into the public interface of this class (they can be used internally, if you like). Instead, I would make a separate add*Listener and remove*Listener methods for different kind of events.

Copy link
Member Author

@jaakkonakaza jaakkonakaza Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think separate add*Listener and remove*Listener methods for different kind of events sounds good, I'll do that

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me too, but in that case I would prefer two sets for primary and secondary listeners respectively, instead of a map (even if only used internally).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, Olli already suggested this in his later comment, but yes, I agree

Copy link
Contributor

Choose a reason for hiding this comment

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

The javadocs for add*Listener could also mention when primary and secondary actions are triggered, since "primary" and "secondary" don't make it obvious

}
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/fi/aalto/cs/apluscourses/ui/exercise/ExercisesView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.intellij.openapi.application.ModalityState;
import fi.aalto.cs.apluscourses.intellij.actions.ActionUtil;
import fi.aalto.cs.apluscourses.intellij.actions.OpenSubmissionAction;
import fi.aalto.cs.apluscourses.intellij.actions.SubmitExerciseAction;
import fi.aalto.cs.apluscourses.presentation.exercise.ExercisesTreeViewModel;
import fi.aalto.cs.apluscourses.ui.GuiObject;
import fi.aalto.cs.apluscourses.ui.base.TreeView;
Expand Down Expand Up @@ -43,7 +44,11 @@ private void createUIComponents() {
exerciseGroupsTree = new TreeView();
exerciseGroupsTree.setCellRenderer(new ExercisesTreeRenderer());
exerciseGroupsTree.addNodeAppliedListener(
"openSubmission",
ActionUtil.createOnEventLauncher(OpenSubmissionAction.ACTION_ID, exerciseGroupsTree));
exerciseGroupsTree.addNodeAppliedListener(
"submitExercise",
ActionUtil.createOnEventLauncher(SubmitExerciseAction.ACTION_ID, exerciseGroupsTree));
}

public TreeView getExerciseGroupsTree() {
Expand Down