Control clicking exercise opens submit window #447

jaakkonakaza · 2021-01-08T14:33:44Z

Description of the PR

Ctrl + left clicking an assignment opens the submission window. Also works when clicking submissions, which I think is fine. Part of #248

Testing

unit	integration	manual
new unit tests created all unit tests pass	new integration tests created all integration tests pass	manual testing went well

Have you updated the TESTING.md or other relevant documentation?

Yes
Not yet. I will do it next.
Not relevant

OlliKiljunen

In general, this looks pretty nice! 👍 I have a couple of concerns I discuss in my comments. See them and tell your thoughts of them! :)

OlliKiljunen · 2021-01-08T14:45:14Z

src/main/java/fi/aalto/cs/apluscourses/ui/base/TreeView.java

+        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);


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.

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

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).

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

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

OlliKiljunen · 2021-01-08T14:47:06Z

src/main/java/fi/aalto/cs/apluscourses/ui/base/TreeView.java

@@ -34,7 +34,7 @@
        }
      };

-  private final transient Set<ActionListener> nodeAppliedListeners = ConcurrentHashMap.newKeySet();
+  private final transient HashMap<String, ActionListener> nodeAppliedListeners = new HashMap<>();


Should probably be ConcurrentHashMap

OlliKiljunen

One thing

OlliKiljunen · 2021-01-08T14:55:38Z

src/main/java/fi/aalto/cs/apluscourses/ui/base/TreeView.java

-  public void addNodeAppliedListener(ActionListener listener) {
-    nodeAppliedListeners.add(listener);
+  public void addNodeAppliedListener(String key, ActionListener listener) {
+    nodeAppliedListeners.put(key, listener);


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.

nikke234

This is a nice addition and a good idea! 👍

Eventually we'll have to figure out how to let users know of this feature, but it is nice to have for "power users" even as is.

Btw, does double-clicking (without ctrl) a submission also open the submission dialog, or does that open the submission in the browser?

jaakkonakaza · 2021-01-08T16:12:01Z

Yes, double clicking submissions still opens them in a browser @nikke234

jaakkonakaza · 2021-01-08T16:17:59Z

Ctrl-clicking a selected assignment doesn't work since it unselects it

sonarqubecloud · 2021-01-08T16:18:13Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

nikke234 · 2021-01-08T16:25:40Z

So if I understood correctly:

ctrl-clicking an unselected assignment opens the submission dialog
ctrl-clicking a selected assignment unselects it
ctrl-clicking a submission opens the submission dialog for the corresponding assignment
double-clicking a submission opens the submission in a browser.

Number 3 and 4 is potentially a weird combination, but then again I think the "submit assignment" icon also works when a submission (instead of an assignment) is selected, so it makes sense.

Number 2 however seems undesirable/unintuitive? I think it would be nice if ctrl-click would always submit, although I wouldn't say it's worth it if it requires significant effort

superseacat · 2021-01-08T19:11:39Z

can you please link as described @jaakkonarhi

nikke234 · 2021-01-10T09:46:36Z

It's related, but this doesn't directly close it, so let's unlink the issue for now.

superseacat · 2021-01-11T08:22:25Z

ctrl-clicking an unselected assignment opens the submission dialog

ctrl-clicking a selected assignment unselects it

ctrl-clicking a submission opens the submission dialog for the corresponding assignment

double-clicking a submission opens the submission in a browser.

I suggest thinking about this approach a bit more... Currently, in modules subtab, ctrl+click regardless of the previous selection state selects and unselects correspondingly, however, allows for multiple selections. And hence, the multiple selections do not make that much sense in assignments subtab, how much do we want to retain the uniform approach between subtabs?

superseacat · 2021-01-11T08:35:13Z

src/main/java/fi/aalto/cs/apluscourses/ui/base/TreeView.java

@@ -151,9 +160,12 @@ public void valueChanged(@NotNull TreeSelectionEvent e) {

    @Override
    public void mouseClicked(@NotNull MouseEvent e) {


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

jaakkonakaza · 2021-01-11T09:43:52Z

I'm starting to feel that the ctrl clicking is a bit useless, since you can submit with a single click with the popup menu, so I'm focusing on that now

superseacat · 2021-01-11T10:24:19Z

I'm starting to feel that the ctrl clicking is a bit useless, since you can submit with a single click with the popup menu, so I'm focusing on that now

let's discuss after the daily!

nikke234 · 2021-01-11T11:26:12Z

I'm starting to feel that the ctrl clicking is a bit useless, since you can submit with a single click with the popup menu, so I'm focusing on that now

Closing this, as we agreed in a meeting that this is the way to go

Control clicking exercise opens submit window

ee80e66

OlliKiljunen reviewed Jan 8, 2021

View reviewed changes

nikke234 reviewed Jan 8, 2021

View reviewed changes

Separate listener sets for different clicks

f3cb133

superseacat linked an issue Jan 8, 2021 that may be closed by this pull request

Exercise list should support submitting using the mouse right-click key #248

Closed

superseacat requested review from superseacat and a team January 9, 2021 10:38

nikke234 removed a link to an issue Jan 10, 2021

Exercise list should support submitting using the mouse right-click key #248

Closed

superseacat reviewed Jan 11, 2021

View reviewed changes

nikke234 closed this Jan 11, 2021

jaakkonakaza deleted the exercise-clicks branch January 11, 2021 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control clicking exercise opens submit window #447

Control clicking exercise opens submit window #447

jaakkonakaza commented Jan 8, 2021

OlliKiljunen left a comment

OlliKiljunen Jan 8, 2021

jaakkonakaza Jan 8, 2021 •

edited

Loading

nikke234 Jan 8, 2021

nikke234 Jan 8, 2021

nikke234 Jan 8, 2021

OlliKiljunen Jan 8, 2021

OlliKiljunen left a comment

OlliKiljunen Jan 8, 2021

nikke234 left a comment

jaakkonakaza commented Jan 8, 2021

jaakkonakaza commented Jan 8, 2021

sonarqubecloud bot commented Jan 8, 2021

nikke234 commented Jan 8, 2021 •

edited

Loading

superseacat commented Jan 8, 2021

nikke234 commented Jan 10, 2021

superseacat commented Jan 11, 2021

superseacat Jan 11, 2021

jaakkonakaza commented Jan 11, 2021

superseacat commented Jan 11, 2021 •

edited

Loading

nikke234 commented Jan 11, 2021

		@@ -151,9 +160,12 @@ public void valueChanged(@NotNull TreeSelectionEvent e) {

		@Override
		public void mouseClicked(@NotNull MouseEvent e) {

Control clicking exercise opens submit window #447

Control clicking exercise opens submit window #447

Conversation

jaakkonakaza commented Jan 8, 2021

Description of the PR

Testing

Have you updated the TESTING.md or other relevant documentation?

OlliKiljunen left a comment

Choose a reason for hiding this comment

OlliKiljunen Jan 8, 2021

Choose a reason for hiding this comment

jaakkonakaza Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

nikke234 Jan 8, 2021

Choose a reason for hiding this comment

nikke234 Jan 8, 2021

Choose a reason for hiding this comment

nikke234 Jan 8, 2021

Choose a reason for hiding this comment

OlliKiljunen Jan 8, 2021

Choose a reason for hiding this comment

OlliKiljunen left a comment

Choose a reason for hiding this comment

OlliKiljunen Jan 8, 2021

Choose a reason for hiding this comment

nikke234 left a comment

Choose a reason for hiding this comment

jaakkonakaza commented Jan 8, 2021

jaakkonakaza commented Jan 8, 2021

sonarqubecloud bot commented Jan 8, 2021

nikke234 commented Jan 8, 2021 • edited Loading

superseacat commented Jan 8, 2021

nikke234 commented Jan 10, 2021

superseacat commented Jan 11, 2021

superseacat Jan 11, 2021

Choose a reason for hiding this comment

jaakkonakaza commented Jan 11, 2021

superseacat commented Jan 11, 2021 • edited Loading

nikke234 commented Jan 11, 2021

jaakkonakaza Jan 8, 2021 •

edited

Loading

nikke234 commented Jan 8, 2021 •

edited

Loading

superseacat commented Jan 11, 2021 •

edited

Loading