Object coordinates R2: rebased from master in 2018 Re #2559, #6078 #8457

josephsl · 2018-06-27T15:52:36Z

Hi,
As requested by @michaelDCurran in #6078:

Link to issue number:

Fixes #2559

Summary of the issue:

Play object coordiantes R2: rebased on 2018 master (see #2559 for details).

Description of how this pull request fixes the issue:

See #6078 discussion history.

Testing performed:

Same as #6078.

Known issues with pull request:

None, although @LeonarddeR raised concerns which should be addressed.

Change log entry:

Same as #6078, although the scope can cover focus movement as well.

…n using object navigation commands (rebased with master in June 2016). re nvaccess#2559

…vObj event. re nvaccess#2559 As object coordinate announcement will be done regardless of focus movement, it was decided to let becomeNavObject event call call object coordinate player function in screen explorer. This means global commands will not use this method (this decisoin was made after talking to Jamie Teh and others).

…rvices. re nvaccess#2559

…ec module (after rebasing with master). re nvaccess#2559.

…access#2559. In NVDA 2017.1, it is possible to hear mouse coordinates on multi-monitor systems. Thus object coordinate announcement function has been modified to take advantage of this. To ensure backward compatibility, minimum screen position paremeter will be a keyword argument, ready to become mandatory if obj coordinate announcement from multi-monitor set ups is desired.

nvaccess#2559

nvaccess#2559. Reviewed by Mick Curran (NV Access)L due to some issues, object brightness won't make sense when playing coordinates, so tell coordinates player function to ignore this. Also updated copyright year.

…nged in 2018.3)

…cess#2559.

…ovided. Re nvaccess#2559. In some cases, obj.location is None, which means TypeError is raised because there's nothing to iterate ovdr. Thus, for cases like this, do not play coordinate beep.

@LeonarddeR

Reviewed by @LeonarddeR: if braille is set to auto-tether and if this is a focused object, tones will not play because the event returns early. To get around this, play the tone first.

josephsl · 2018-06-27T15:56:05Z

Hi,

In addition to rebasing on master, it also removes mouseHandler.playAudioCoordinates function as no other module uses this function in Core. Mouse handler itself will call screen explorer function.

Thanks.

source/NVDAObjects/__init__.py

LeonarddeR · 2018-06-28T05:52:04Z

source/gui/settingsDialogs.py

@@ -1487,10 +1487,16 @@ def makeSettings(self, settingsSizer):

 		# Translators: This is the label for a checkbox in the
 		# object presentation settings panel.
+		objCoordinatesText = _("&Play object coordinates"))
+		self.playObjectCoordinatesCheckBox=sHelper.addItem(wx.CheckBox(self,label=objCoordinatesText))


Even if the implementation in becomeNavigatorObject stays as it is, I'd prefer not to have coordinates played for focus changes. May be it is a good idea to make this option a combo box, as you suggested earlier.

LeonarddeR · 2018-06-28T06:31:52Z

source/screenExplorer.py

+	@type obj: int
+	"""
+	# Have a fake wx.Point in case min pos is not defined on multiple monitors.
+	if screenMinPos is None:


Why should this be allowed? See my comment below about the way you calculate the screen width and height.

Hi,

This came about due to wx.Point fix in order to support mouse tracking on multiple monitors (Reef knows more about this than I do).

Thanks.

May be we should step away from getting this information from wx and use GetSystemMetrics instead?

To clarify, see the SM_CXVIRTUALSCREEN, SM_CYVIRTUALSCREEN, SM_XVIRTUALSCREEN and SM_YVIRTUALSCREEN. If I understand the code correctly (and probably I don't), throwing these four constants at GetSystemMetrics would make mouseHandler.getTotalWidthAndHeightAndMinimumPosition obsolete.

LeonarddeR · 2018-06-28T06:39:23Z

source/screenExplorer.py

+	if screenMinPos is None:
+		screenMinPos = wx.Point()
+	# Do not play any tone if offscreen.
+	if 0 <= x <= screenWidth or 0 <= y <= screenHeight:


It is a matter of style, but personally I'd prefer this to return early.

Hi,

Ah, the chained comparison: my message in that fragment was that it should play coordinate beep as long as x and y are referring to things on screen. It was done like this because in browse mode, things will show up offscreen.

Thanks.

Yeah, I would also prefer that the logic in this if statement was reversed and just had an early return. This reduces the indentation level of everything below.

LeonarddeR · 2018-06-28T06:41:43Z

source/screenExplorer.py

+			startY=min(max(y-blurFactor,0),screenHeight)+screenMinPos.y
+			width=min(blurFactor+1,screenWidth)
+			height=min(blurFactor+1,screenHeight)
+			grey=screenBitmap.rgbPixelBrightness(mouseHandler.scrBmpObj.captureImage( startX, startY, width, height)[0][0])


Could you move the scrBmpObj to screenExplorer as well? It currently makes no sense within the scope of mouseHandler.

Hi,

Hmmm, the object itself is initialized and nullified when mouse handler starts and ends, respectively. We need to gather more feedback regarding this, as it could break features if this is moved without careful thinking. Thanks.

There are no other references to scrBmpObj outside of mouseHandler.py, you can confirm with git grep -i scrBmpObj.

Are you worried that addons are using it? My preference would be to understand its usage, and what the intention was behind initialising it this way, with the goal of moving it out of mouseHandler

LeonarddeR · 2018-06-28T06:50:54Z

source/screenExplorer.py

+	l,t,w,h=obj.location
+	x = l+(w/2)
+	y = t+(h/2)
+	screenWidth, screenHeight = api.getDesktopObject().location[-2:]


This was actually the main concern I mentioned earlier. Could you please bring this in line with how mouseHandler does this (i.e. use mouseHandler.getTotalWidthAndHeightAndMinimumPosition)

I think it makes sense to move some functionality to a new module. May be locationHelper could be a fit for this. I propose at least moving the following functions, as they aren't specific to the mouseHandler.

getTotalWidthAndHeightAndMinimumPosition

getMinMaxPoints

Hi,

Code reorg: I propose putting this as part of Python 3 transition.

Thanks.

LeonarddeR · 2018-06-28T06:51:39Z

source/screenExplorer.py

+	screenWidth, screenHeight = api.getDesktopObject().location[-2:]
+	try:
+		playLocationCoordinates(x, y, screenWidth, screenHeight, detectBrightness=False)
+	except AttributeError:


What could cause the AttributeError here?

Hi,

Bad assumptions from my part: it was added because it was originally part of mouse handler. Testing shows this is no needed anymore.

Thanks for catching this and helping me update my assumptions.

@LeonarddeR

Reviewed by @LeonarddeR (Babbage) and catching log errors: * screenExplorer.playObjectCoordinates: catching attribute error is no longer necessary (born from old assumptions as the code was part of mouse handler at first). * gui/settingsDialogs: remove extra right parenthesis from object coordinate control label.

@LeonarddeR

Reviewed by @LeonarddeR (Babbage): make object coordinate announcement a combo box: * Config spec: object coordinates is now an option list. * Settings: checkbox -> combo box, using same logic as progress bar output setting. * NVDAObbjects/event_becomeNavigatorObject: detect different coordinate announcement settings and play coordinate beep if appropriate (for example, only play beeps if focus has moved and is told to play it when focus moves or both object navigation and focus).

…oggle this setting. Re nvaccess#2559.

…vaccess#2559.

@LeonarddeR

Reviewed by @LeonarddeR 9Babbage): pull in multiple displays support from mouse handler. This reolsves a potential problem where coordinates will not be played if moving to an object located on secondary displays.

josephsl · 2018-06-28T21:05:13Z

Hi,

To @LeonarddeR: hope I addressed your comments. Thanks.

LeonarddeR · 2018-07-20T17:04:30Z

source/globalCommands.py

@@ -1463,6 +1463,29 @@ def script_toggleProgressBarOutput(self,gesture):
 	script_toggleProgressBarOutput.__doc__=_("Toggles between beeps, speech, beeps and speech, and off, for reporting progress bar updates")
 	script_toggleProgressBarOutput.category=SCRCAT_SPEECH

+	def script_togglePlayObjectCoordinates(self,gesture):


Could you please change this implementation to follow the implementation strategy of script_braille_toggleFocusContextPresentation? This includes creating constants for objFocus, objNav, both, etc.

Alternatively, you can choose the implementation of script_toggleReportLineIndentation.

I agree, perhaps define a dictionary first with the expected values and the ui messages, then you can get the index of the current playObjectCoordinates value increment it, and fetch the ui message from the dictionary for the new playObjextCoordinates value.

josephsl · 2019-02-22T01:17:58Z

Hi,

I'm willing to yield this pull request to higher priority work. in the meantime, I'll generate a try build for adventurers to test with.

Thanks.

josephsl · 2019-08-14T06:29:52Z

Hi,,
Six months later...

That high-priority work (Python 3) is done, so I'm coming back to this. I'll do something about this PR this week.

Thanks.

AppVeyorBot · 2019-09-18T20:16:22Z

PR introduces Flake8 errors 😲

See test results for Failed build of commit abbd8791b5

Lint work include: * Global commands: convert the script to use script decorator. * Spacing around operators. * Split long lines.

josephsl · 2020-04-14T19:36:37Z

Hi,

Being considered for abandoning it in light of #11006.

Thanks.

feerrenrut · 2020-04-15T10:54:36Z

Being considered for abandoning it in light of #11006.

If you are no longer interested in making this change, please do say so. Otherwise please continue to work on this PR. #11006 requests no new PR's. This PR already exists and is thus in the backlog of PR's that we want to address before accepting new ones. We are trying to stop PR's from going stale, and then becoming hard to merge.

Forgive me for replying to you multiple times, I want to avoid the community being mislead.

josephsl · 2020-04-15T10:56:20Z

I’ll do another rebase attempt and see if it is feasible to continue this work.

AppVeyorBot · 2020-04-15T11:13:29Z

System test failure
Build (for testing PR)

See test results for failed build of commit fb09f17911

josephsl · 2020-05-06T06:01:57Z

Hi,

Update: yep, the PR is back. At least until coordinates-based audio themes comes to NVDA (there is an add-on out there already), I think this PR would serve useful purposes.

Thanks.

AppVeyorBot · 2020-05-06T06:07:55Z

Build (for testing PR)

See test results for failed build of commit 1bef9c061f

…definition.

feerrenrut · 2020-07-03T06:56:21Z

We have discussed this and have decided that although interesting it would be better as an addon to explore the user experience. If necessary, we would accept a PR to enable support for an addon such as this.

josephsl · 2020-07-03T07:03:26Z

Hi, the add-on is already available: Object Location Tones, or better yet, Audio Themes 3D. Thanks.

josephsl added 16 commits June 27, 2018 08:28

Object coordinates: It is now possible to hear object coordinates whe…

e632ebd

…n using object navigation commands (rebased with master in June 2016). re nvaccess#2559

Object coordinates: obj coordinates checkbox now follows guiHelper se…

59fbfa6

…rvices. re nvaccess#2559

Object coordinates: updated copyright headers, removed unused imports.

b74d6ba

Config: play obj coordinates validator now lives in the new config sp…

9ed36d5

…ec module (after rebasing with master). re nvaccess#2559.

Object coordinates: do not play the tone if the object ia offscreen. re

347afb9

nvaccess#2559

User guide: add anchors for object coordinate checkbox. re nvaccess#2559

a7923df

Update copyright years

040b013

Object coordinates: do not detect brightness, copyright year update. Re

08a0283

nvaccess#2559. Reviewed by Mick Curran (NV Access)L due to some issues, object brightness won't make sense when playing coordinates, so tell coordinates player function to ignore this. Also updated copyright year.

Mouse handler: update copyright years

6545ef9

Mouse handler: set copyright header to that of NVDA 2018.2 (to be cha…

946087d

…nged in 2018.3)

Mouse handler: restore removed imports.

3780fa4

User guide: clarify the scope of object coordinates checkbox. Re nvac…

eed2f8f

…cess#2559.

Screen explorer/object coordinates: return early if no location is pr…

f3199df

…ovided. Re nvaccess#2559. In some cases, obj.location is None, which means TypeError is raised because there's nothing to iterate ovdr. Thus, for cases like this, do not play coordinate beep.

Object coordinates: play coordinate tone first. Re nvaccess#2559.

da52142

Reviewed by @LeonarddeR: if braille is set to auto-tether and if this is a focused object, tones will not play because the event returns early. To get around this, play the tone first.

josephsl requested a review from michaelDCurran June 27, 2018 15:53

LeonarddeR suggested changes Jun 28, 2018

View reviewed changes

josephsl added 6 commits June 28, 2018 09:42

Object coordinates: add an unassigned command in global commands to t…

61c92b7

…oggle this setting. Re nvaccess#2559.

User guide: document the redesigned object coordinates combo box. Re n…

c911775

…vaccess#2559.

Mouse handler: remove blank line

d3b479b

Object coordinates/screen explorer: address review actions.

a971550

Reviewed by @LeonarddeR 9Babbage): pull in multiple displays support from mouse handler. This reolsves a potential problem where coordinates will not be played if moving to an object located on secondary displays.

josephsl added 3 commits July 17, 2018 23:24

Merge branch 'master' into i2559

6735c13

Object coordinates setting: log message typo fix

7432daf

Merge branch 'master' into i2559

1b95725

LeonarddeR reviewed Jul 20, 2018

View reviewed changes

josephsl requested a review from LeonarddeR February 22, 2019 01:16

josephsl added 2 commits April 1, 2019 14:13

Merge branch 'master' into i2559

d8c0690

Merge branch 'master' into i2559

5bbe68e

LeonarddeR added the AfterPython3Transition label Apr 24, 2019

feerrenrut removed the AfterPython3Transition label May 8, 2019

Merge branch 'master' into i2559

7743304

Merged master

45edb7c

josephsl added 2 commits September 18, 2019 15:53

Screen explorer/Python 3: xrange -> range. Re nvaccess#2559

7cfc026

Object coordinates: lint work. Re nvaccess#2559.

7dd0f17

Lint work include: * Global commands: convert the script to use script decorator. * Spacing around operators. * Split long lines.

LeonarddeR mentioned this pull request Oct 16, 2019

Object location tones #10390

Closed

Merged master

bc25e5d

feerrenrut added component/audio-ux feature labels Apr 8, 2020

Merge branch 'master' into i2559

06e3dfd

dpy013 mentioned this pull request Apr 23, 2020

Touch interaction: allow users to disable touch support completely #10557

Merged

Merge branch 'master' into i2559

aa8b853

Object coordinates script: move translator comment below description …

0def3b0

…definition.

feerrenrut closed this Jul 3, 2020

josephsl deleted the i2559 branch July 3, 2020 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Object coordinates R2: rebased from master in 2018 Re #2559, #6078 #8457

Object coordinates R2: rebased from master in 2018 Re #2559, #6078 #8457

josephsl commented Jun 27, 2018 •

edited by ehollig

Loading

josephsl commented Jun 27, 2018

LeonarddeR Jun 28, 2018

LeonarddeR Jun 28, 2018

josephsl Jun 28, 2018

LeonarddeR Jul 20, 2018

LeonarddeR Jul 20, 2018

LeonarddeR Jun 28, 2018

josephsl Jun 28, 2018

feerrenrut Jul 31, 2018

LeonarddeR Jun 28, 2018

josephsl Jun 28, 2018

feerrenrut Jul 31, 2018

LeonarddeR Jun 28, 2018

josephsl Jun 28, 2018

LeonarddeR Jun 28, 2018

josephsl Jun 28, 2018

josephsl commented Jun 28, 2018

LeonarddeR Jul 20, 2018

feerrenrut Jul 31, 2018

josephsl commented Feb 22, 2019

josephsl commented Aug 14, 2019

AppVeyorBot commented Sep 18, 2019

josephsl commented Apr 14, 2020

feerrenrut commented Apr 15, 2020

josephsl commented Apr 15, 2020 via email •

edited by feerrenrut

Loading

AppVeyorBot commented Apr 15, 2020

josephsl commented May 6, 2020

AppVeyorBot commented May 6, 2020

feerrenrut commented Jul 3, 2020

josephsl commented Jul 3, 2020 via email •

edited by feerrenrut

Loading

Object coordinates R2: rebased from master in 2018 Re #2559, #6078 #8457

Object coordinates R2: rebased from master in 2018 Re #2559, #6078 #8457

Conversation

josephsl commented Jun 27, 2018 • edited by ehollig Loading

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

josephsl commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephsl commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephsl commented Feb 22, 2019

josephsl commented Aug 14, 2019

AppVeyorBot commented Sep 18, 2019

josephsl commented Apr 14, 2020

feerrenrut commented Apr 15, 2020

josephsl commented Apr 15, 2020 via email • edited by feerrenrut Loading

AppVeyorBot commented Apr 15, 2020

josephsl commented May 6, 2020

AppVeyorBot commented May 6, 2020

feerrenrut commented Jul 3, 2020

josephsl commented Jul 3, 2020 via email • edited by feerrenrut Loading

josephsl commented Jun 27, 2018 •

edited by ehollig

Loading

josephsl commented Apr 15, 2020 via email •

edited by feerrenrut

Loading

josephsl commented Jul 3, 2020 via email •

edited by feerrenrut

Loading