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

add reportActiveConfigurationProfile command #9325

Merged
merged 4 commits into from Apr 14, 2020

Conversation

feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Feb 25, 2019

Link to issue number:

Initially developed due to #9239 (comment)

Summary of the issue:

Even though the issue of announcing the current profile was resolved in #9239 reporting the current profile name may be useful more generally.

Description of how this pull request fixes the issue:

A new command is introduced that allows the active configuration profile to be reported.
Reports "normal configuration profile active" or "{profileName} configuration profile active"
No default gesture is assigned.

Testing performed:

Checked that the correct name is reported when a configuration profile is active, and when the normal configuration is active.

Known issues with pull request:

Change log entry:

New Features
- Added a command to report the active configuration profile (#9325).

@DrSooom
Copy link

DrSooom commented Feb 26, 2019

def script_reportActiveConfigurationProfile(self, gesture):

"(self, gesture):" » "(self,gesture):" (See a few lines below)

What global input gesture is used for this command? NVDA+CTRL+Shift+P would be possible. And I guess that the message output will be via speech and braille together, right?

@leonardder
Copy link
Collaborator

leonardder commented Feb 26, 2019

You could consider assigning shift+NVDA+p as default.

@feerrenrut
Copy link
Member Author

feerrenrut commented Feb 26, 2019

"(self, gesture):" » "(self,gesture):" (See a few lines below)

Visually, code without spaces is harder to read. So while much of the code is formatted this way, though certainly not all, I hope we are able to start to transition.

And I guess that the message output will be via speech and braille together, right?

Correct, although I have not tested with a braille display.

You could consider assigning shift+NVDA+p as default.

Given the revised approach to #9325 (not changing the title text), this command might not be so urgent. If there is demand for it, we could still consider including it. However I think we should be cautious about assigning a default gesture.

@DrSooom
Copy link

DrSooom commented Feb 27, 2019

@feerrenrut: Not only visually, also on a braille display. As I'm not familiar with Python, I didn't know that both methods are valid and equal. I thought it was a typo. Thanks for the explanation.

@JulienCochuyt
Copy link
Collaborator

JulienCochuyt commented Jul 15, 2019

You could consider assigning shift+NVDA+p as default.

As @DrSooom proposed in #9325 (comment) I think NVDA+control+shift+p is more relevant as it would:

Btw, I am not at all in favor of unassigned default gestures for commands that can be useful for beginners to intermediate users.

@feerrenrut
Copy link
Member Author

feerrenrut commented Apr 6, 2020

The issue this PR attempted to address was resolved in the PR. However I think it could still be useful functionality, so I'll mark it as ready for review. Since sensible gestures are short supply I'd prefer to leave this unbound initially. I don't think that working with profiles is a beginner feature.

@feerrenrut feerrenrut marked this pull request as ready for review Apr 6, 2020
source/globalCommands.py Outdated Show resolved Hide resolved
@leonardder
Copy link
Collaborator

leonardder commented Apr 6, 2020

@lukaszgo1 has a good point.

@feerrenrut feerrenrut requested a review from leonardder Apr 7, 2020
@AppVeyorBot

This comment has been minimized.

@feerrenrut
Copy link
Member Author

feerrenrut commented Apr 14, 2020

I have addressed the changes. The core concept was already reviewed. I'm going to merge this PR.

@feerrenrut feerrenrut merged commit da6d460 into master Apr 14, 2020
1 check passed
@feerrenrut feerrenrut deleted the addReportCurrentConfigurationProfile branch Apr 14, 2020
@nvaccessAuto nvaccessAuto added this to the 2020.1 milestone Apr 14, 2020
@feerrenrut feerrenrut modified the milestones: 2020.1, 2020.2 Apr 14, 2020
feerrenrut added a commit that referenced this pull request Apr 14, 2020
@Adriani90
Copy link
Collaborator

Adriani90 commented Apr 14, 2020

is this documented accordingly in the user guide?

@feerrenrut
Copy link
Member Author

feerrenrut commented Apr 16, 2020

@Adriani90 Are all commands documented in the user guide? It's not a particularly interesting command in need of explanation, and is listed in the gestures dialog.

@Adriani90
Copy link
Collaborator

Adriani90 commented Apr 16, 2020

@feerrenrut
Copy link
Member Author

feerrenrut commented Apr 16, 2020

Then we have two places to maintain the list of commands, and two places to keep translations consistent. I don't think adding something to the user guide is a good way to make a new feature discoverable, it's main purpose is to explain confusing or complicated options. The changes file was updated, its purpose is discoverability. Otherwise, for a listing of all commands users should consult the inputGestures dialog. To specifically make unassigned commands more discoverable, we should enhance the input gestures dialog with filters.

@Adriani90
Copy link
Collaborator

Adriani90 commented Apr 16, 2020

@feerrenrut I agree. If there is a filter implemented, then it would help to have a cathegory which displays only the unassigned gestures for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants