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.

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

#1875 #2008

Open
wants to merge 91 commits into
base: master
Choose a base branch
from
Open

Conversation

spica198
Copy link
Collaborator

@spica198 spica198 commented Apr 10, 2024

Prerequisites

  • Reviewed the checklist

  • Reviewed feedback from the "Sonar Cloud" bot. Note that you have to wait
    for the "CI / Unit Tests") to complete first. Failed Unit tests can be
    debugged by adding the label "verbose logging" to the GitHub PR.

Description of the Change

In the Data Access View, map keyboard shortcuts to templates. Default is from ctrl-1 to ctrl-5 and after that user can provide shortcut of their choice

Alternate Designs

Why Should This Be In Core?

Benefits

Possible Drawbacks

Verification Process

  1. Save a template in Data acccess view with keyboard shortcut
  2. Should be able to load template by pressing keyboard shortcut

Applicable Issues

#1875

@spica198 spica198 linked an issue Apr 10, 2024 that may be closed by this pull request
1 task
@spica198 spica198 requested a review from OrionsGuardian April 10, 2024 00:01
public class RecordKeyboardShortcut {

public Optional<String> start(Stage primaryStage) {
var label = new Label();
Copy link
Collaborator

Choose a reason for hiding this comment

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

final

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we also declare as a Label rather than var

}

private KeyCombination createCombo(KeyEvent event) {
var modifiers = new ArrayList<Modifier>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

final

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we declare as List here?

@OrionsGuardian
Copy link
Collaborator

image
Each new template that I save has a Ctrl 1 prefix attached to it.
How do we set a differently numbered shortcut when saving the template ?
Pressing Ctrl 1 does correctly activate the first entry that has Ctrl 1 as it's prefix.

public class RecordKeyboardShortcut {

public Optional<String> start(Stage primaryStage) {
var label = new Label();
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we also declare as a Label rather than var

}

private KeyCombination createCombo(KeyEvent event) {
var modifiers = new ArrayList<Modifier>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we declare as List here?

@spica198
Copy link
Collaborator Author

image Each new template that I save has a Ctrl 1 prefix attached to it. How do we set a differently numbered shortcut when saving the template ? Pressing Ctrl 1 does correctly activate the first entry that has Ctrl 1 as it's prefix.

@spica198 spica198 closed this Apr 16, 2024
@spica198 spica198 reopened this Apr 16, 2024
@OrionsGuardian
Copy link
Collaborator

The Additional Information section of ticket #1875 suggests the functionality should allow the user to assign the shortcuts to the templates they want to use.
Currently, it will assign shortcuts (1 through to 5) to the first 5 templates saved.
Saving future templates won't assign any further shortcuts until one of the previous templates are deleted.

The functionality should allow the user to select a shortcut number when saving a template, and also reassign/remove shortcut numbers when loading templates.

@OrionsGuardian
Copy link
Collaborator

The core functionality now seems to be working fine, but it feels like too many popup dialogs appearing to achieve this.
I've mocked up a sample alternative layout:

image

So you can optionally set a shortcut, where it pops up the dialog to get the keystroke, then returns to this Saving dialog.
Use a warning symbol (!) to indicate that the chosen Shortcut is already being used.
The orange warning message can either be shown as a line in the dialog, or set as a tooltip for the warning symbol.
Then, when you click OK it uses the shortcut (if one was chosen), and removes the shortcut from any old template that was using it.

@OrionsGuardian OrionsGuardian added the verbose-logging test code in CI with verbose output label Jul 17, 2024
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
72.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@@ -1,5 +1,5 @@
/*
* Copyright 2010-2024 Australian Signals Directorate
* Copyright 2010-2021 Australian Signals Directorate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update copyright to 2025

@@ -0,0 +1,114 @@
/*
* Copyright 2010-2024 Australian Signals Directorate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update copyright to 2025

@@ -182,6 +191,22 @@ public static void saveJsonPreferences(final Optional<String> saveDir, final Opt
}
}

public static Optional<String> getDefaultKeyboardShortcut(File preferenceDirectory) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

parameter final

@@ -0,0 +1,136 @@
/*
* Copyright 2010-2024 Australian Signals Directorate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update copyright year

import org.apache.commons.lang3.StringUtils;

/**
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can a description of what the class does be added?

@@ -0,0 +1,130 @@
/*
* Copyright 2010-2024 Australian Signals Directorate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update copyright year

import org.apache.commons.lang3.StringUtils;

/**
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add description for the class

}

//For unit test
public RecordKeyboardShortcut(KeyPressLabelDialog td) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

parameter final

@@ -0,0 +1,266 @@
/*
* Copyright 2010-2024 Australian Signals Directorate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update copyright year

import org.apache.commons.lang3.StringUtils;

/**
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add description for class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
verbose-logging test code in CI with verbose output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants