Remove RTL and LTR marks from taskbar clock text and from edit boxes in a properties window by lukaszgo1 · Pull Request #8643 · nvaccess/nvda

lukaszgo1 · 2018-08-17T17:32:46Z

Link to issue number:

Summary of the issue:

Right-to-left and left-to-right characters In the taskbar clock and in the read-only edit boxes in a properties window was annoying when reading character by character, or when displayed on a braille display.

Description of how this pull request fixes the issue:

The characters are removed from clock name and value, from read-only edit boxes in Windows Explorer,, and from read-only edit boxes in dllhost, which is responsible for displaying properties window under older builds of Windows 10.

Testing performed:

Tested as follows on Windows versions from Win 7 to the latest insider build with a braille display connected:

Moved to the clock icon in the system tray
Ensured that the role and value are reported only once to avoid double announcement of clock icon in the system tray #4364 again.
With the review cursor moved character by character to ensure that RTL and LTR marks are gone from the clock text.
Opened properties window for a folder or file, moved with the object navigation to the modified edit box, and ensured that the RTL and LTR marks are no longer present.

The test build was also tested by @DrSooom, and he also confirms, that the issue is fixed.

Known issues with pull request:

I’ve removed Right-to-left character from the clock, however I haven’t seen it there. Removing it doesn’t hurt, and for right-to-left languages it might be present.
The RTL and LTR marks are defined three times in the code. It will be nice to clean this up, but I have no idea where to place them to avoid this duplication.

Change log entry:

The removal from the clock icon was covered by #5729 so only remaining thing is

Section: Bug fixes

NVDA no longer reports (LTR and RTL marks) in Braille or per-character speech when accessing the properties window.

Remove from clock name and value, read-only edit boxes in the properties window Add an app module for dllhost, which is used to remove those characters from properties window on older Windows 10 builds.

lukaszgo1 · 2018-08-18T09:37:08Z

@dkager Would you be able to reviev this?

…t compatible with both Python 2 and 3.

lukaszgo1 · 2018-09-23T18:52:03Z

@dkager Ping

dkager · 2018-09-23T18:56:00Z

I thought I had already reviewed the latest version, but apparently not. Will do tomorrow.

dkager

A good patch, but still a few comments.

dkager · 2018-09-25T17:26:09Z

 class TrayClockWClass(IAccessible):
 	"""
 	Based on NVDAObject but the role is changed to clock.
+	Depending on the version of Windows name or value contains left-to-right or right-to-left  characters, so  remove them from both.


Some double spaces in this comment.

dkager · 2018-09-25T17:28:14Z

 		# #4364 On some versions of Windows name contains redundant information that is available either in the role or the value, however on Windows 10 Anniversary Update and later the value is empty, so we cannot simply dismiss the name.
 		if super(TrayClockWClass, self).value is None:
-			return super(TrayClockWClass, self).name
+			return super(TrayClockWClass, self).name.replace(CHAR_LTR_MARK,'').replace(CHAR_RTL_MARK,'')


Coming at this fresh I think I would put the replacement on its own line, e.g. create a variable holding the name, then a chained replace combined with the return on the next line. I don't know if we recommend a maximum line length though, maybe I'm being fussy.

dkager · 2018-09-25T17:30:12Z

 		return None

+	def _get_value(self):
+		if super(TrayClockWClass, self).value is None:


Again, I would collect the value in its own local variable, perform the replacement only if it is not NOne and finally return the variable.

dkager · 2018-09-25T17:32:18Z

+# This file may be used under the terms of the GNU General Public License, version 2 or later.
+# For more details see: https://www.gnu.org/licenses/gpl-2.0.html
+
+""" Under older builds of Windows 10 (from RTM release to  Creators Update) dllhost is used to display a properties window. 


Double space in comment.

dkager · 2018-09-25T17:32:42Z

+
+""" Under older builds of Windows 10 (from RTM release to  Creators Update) dllhost is used to display a properties window. 
+Read-Only edit boxes in it can contain dates that include unwanted left-to-right and right-to-left indicator characters.
+This simply imports a proper class from explorer app module, and maps it to a edit control.


"from explorer app module" --> "from the explorer app module"

dkager · 2018-09-25T17:34:16Z

+
+	def chooseNVDAObjectOverlayClasses(self, obj, clsList):
+		windowClass = obj.windowClassName
+


This function is so short that I wouldn't put blank line here.

dkager · 2018-09-25T17:34:47Z

+	def chooseNVDAObjectOverlayClasses(self, obj, clsList):
+		windowClass = obj.windowClassName
+
+		if windowClass == "Edit":


But can we be sure that this is read-only? Should you not check the state too?

Yes we can, because in explorer.py i am removing RTL and LTR chars only when edit has readonly in states. I've done it this way to avoid additional check in both explorer.py and dllhost.py. The same goes for your last commend.

My bad for not seeing this. @LeonarddeR what do you think?

dkager · 2018-09-25T17:35:24Z

+
+		if windowClass == "Edit":
+			clsList.insert(0, ReadOnlyEditBox)
+			return


This return statement is redundant because the function ends here anyway.

dkager · 2018-09-25T17:37:26Z

+#These can contain dates that include unwanted left-to-right and right-to-left indicator characters.
+
+	def _get_windowText(self):
+		if  super(ReadOnlyEditBox, self).windowText is not None and controlTypes.STATE_READONLY in self.states:


Same comment as with the replacements above: this code can be made a bit more readable.

dkager · 2018-09-25T17:38:28Z

 					clsList.insert(0, NotificationArea)
+			return 
+
+		if windowClass == "Edit":


Can we be sure this is read-only?

lukaszgo1 · 2018-09-28T10:31:03Z

@dkager I believe I've addressed all you commends.

dkager · 2018-09-29T17:58:40Z

-			return super(TrayClockWClass, self).name
-		return None
+	# #4364 On some versions of Windows name contains redundant information that is available either in the role or the value, however on Windows 10 Anniversary Update and later the value is empty, so we cannot simply dismiss the name.
+		ClockValue = super(TrayClockWClass, self).value


I would refactor this to:

if super(TrayClockWClass, self).value is None: clockName = super(TrayClockWClass, self).name return clockName.replace(CHAR_LTR_MARK,'').replace(CHAR_RTL_MARK,'') return None

Note the lower-case c in clockName.

dkager · 2018-09-29T17:59:47Z

+		return ClockName
+
+	def _get_value(self):
+		ClockValue = super(TrayClockWClass, self).value


Please follow the naming conventions for variables, i.e. clockValue.

dkager · 2018-09-29T18:00:42Z

 	(None,oleacc.ROLE_SYSTEM_ALERT):"Dialog",
 	(None,oleacc.ROLE_SYSTEM_PROPERTYPAGE):"Dialog",
 	(None,oleacc.ROLE_SYSTEM_GROUPING):"Groupbox",
-	("TrayClockWClass",oleacc.ROLE_SYSTEM_CLIENT):"TrayClockWClass",


Are you sure this will not break older versions of Windows?

Yes I am. I've tested under Windows 7, 8, 8.1 and all released builds of Windows 10.

dkager · 2018-09-29T18:02:00Z

+
+	def chooseNVDAObjectOverlayClasses(self, obj, clsList):
+		windowClass = obj.windowClassName
+		if windowClass == "Edit":


I don't think you addressed the question I had about this line.

Yes i do. See {this commend

dkager · 2018-09-29T18:02:39Z

+#These can contain dates that include unwanted left-to-right and right-to-left indicator characters.
+
+	def _get_windowText(self):
+		WindowText = super(ReadOnlyEditBox, self).windowText


Please follow the naming convention for variables, i.e. windowText.

dkager · 2018-09-29T18:03:08Z

 					clsList.insert(0, NotificationArea)
+			return 
+
+		if windowClass == "Edit":


I don't think you addressed the question I had about this line.

lukaszgo1 · 2018-10-08T19:45:46Z

@dkager All your commends are now addressed.

…not present in the name.

lukaszgo1 · 2018-10-18T10:20:28Z

@LeonarddeR Can you please respod to this @dkager s question?

lukaszgo1 · 2018-12-02T22:27:23Z

@dkager @LeonarddeR Can one of your please review this and either approve, or request further changes? I'd really like to have it includet in 2019.1, mainly because it fixes a bug which is according to the change log fixed in 2018.2.

Adriani90 · 2018-12-03T23:04:33Z

cc: @michaelDCurran

dpy013 · 2019-02-14T10:23:40Z

@dkager @LeonarddeR Can one of your please review this and either approve, or request further changes? I'd really like to have it includet in 2019.1, mainly because it fixes a bug which is according to the change log fixed in 2018.2.
I also hope that this PR can appear in the 2019.1 version.

dkager · 2019-02-15T17:39:05Z


+	def _get_value(self):
+		clockValue = super(TrayClockWClass, self).value
+		if not clockValue is None:


I think the convention is to write
if clockValue is not None:

lukaszgo1 · 2019-02-15T20:27:12Z

@dkager All done.

dpy013 · 2019-02-16T08:39:34Z

@dkager All done.
This is good news.

feerrenrut · 2019-03-26T09:28:38Z

I think the main remaining concern is the duplication of the constants for the LTR and RTL mark characters. I would recommend putting these in a module eg (source/unicodeCharConsts.py).

Using git grep "'"'\\u' (in Git bash) shows some other candidates for this:
u'\u3000':'IDEOGRAPHIC SPACE' some of these strings have a space appended.... I haven't inspected the code but I wonder if this a mistake. Alternatively, the goal may be to replace 'IDEOGRAPHIC SPACE' or regular space.
u'\ufffc':OBJECT REPLACEMENT CHARACTER
u'\u0007':BELL

If this turns out to be difficult, then I don't think this should hold up this PR.

feerrenrut

Thanks for your work on this! I'm happy with the changes here.

I noticed in one comment you mentioned you had tested this across different versions of windows? Could you please update the PR description with this information. While it may be obvious to some people, could you also add step by step instructions for testing this to the testing section? It makes it easier for others to understand how you have tested it, perhaps they have other ideas.

I won't merge this immediately, I would like to hear what you think about moving the character constants out to their own module.

lukaszgo1 · 2019-03-26T13:55:54Z

@feerrenrut I've updated an initial commend with the steps to test this. Is this sufficient? Regarding moving the unicode chars to the separate file it would make source code cleaner, but testing of this might be difficult.
I would rather merge this as is, and then move the unicode symbols in a separate PR ,because these two changes aren't directly related and moving some of these symbols might introduce regressions.

feerrenrut · 2019-03-27T13:49:04Z

Thanks @lukaszgo1, I'll merge this in.

Remove RTL and LTR marks from various places. Fix for nvaccess#8361

40f82e6

Remove from clock name and value, read-only edit boxes in the properties window Add an app module for dllhost, which is used to remove those characters from properties window on older Windows 10 builds.

lukaszgo1 added 2 commits August 22, 2018 11:06

Merge branch 'master' into I8361

2063dc9

Modified syntacs of import statement in dllhost app module, to make i…

97c01b5

…t compatible with both Python 2 and 3.

dkager suggested changes Sep 25, 2018

View reviewed changes

lukaszgo1 added 2 commits September 28, 2018 11:33

Merge branch 'master' into I8361

7c63dce

Reviev actions

a88804d

dkager suggested changes Sep 29, 2018

View reviewed changes

Review actions 2

937bf6e

Change role of the clock icon to clock only when the word 'clock' is …

f42e558

…not present in the name.

lukaszgo1 added 2 commits December 5, 2018 16:00

Merge branch 'master' into I8361

5315728

Merge branch 'master' into I8361

73abf2c

dkager suggested changes Feb 15, 2019

View reviewed changes

lukaszgo1 added 2 commits February 15, 2019 21:20

Merge branch 'master' into I8361

4ea0d45

Review actions

4838399

dkager approved these changes Feb 15, 2019

View reviewed changes

lukaszgo1 added 2 commits March 8, 2019 20:03

Merge branch 'master' into I8361

dec4896

Merge branch 'master' into I8361

16a04fb

feerrenrut approved these changes Mar 26, 2019

View reviewed changes

feerrenrut merged commit 32a0f22 into nvaccess:master Mar 27, 2019

nvaccessAuto added this to the 2019.1 milestone Mar 27, 2019

feerrenrut added a commit that referenced this pull request Mar 27, 2019

Update changes file for PR #8643

89f0702

michaelDCurran modified the milestones: 2019.1, 2019.2 Mar 27, 2019

lukaszgo1 deleted the I8361 branch May 13, 2019 15:56

cary-rowen mentioned this pull request Jun 19, 2019

NVDA Chinese input error #9772

Closed

lukaszgo1 mentioned this pull request Jun 20, 2019

Hopefully no longer freeze NVDA when Chinese input method is in use in Windows Explorer #9779

Merged

lukaszgo1 mentioned this pull request Jun 17, 2020

NVDA announces document replacement character for Firefox documents #11177

Closed

This was referenced Sep 5, 2023

Edit controls in explorer fail when they have a native UIA implementation #15375

Closed

Fix issues with UIA (Rich)Edit implementations #15378

Closed


		def chooseNVDAObjectOverlayClasses(self, obj, clsList):
		windowClass = obj.windowClassName

Uh oh!

Conversation

lukaszgo1 commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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:

Uh oh!

lukaszgo1 commented Aug 18, 2018

Uh oh!

lukaszgo1 commented Sep 23, 2018

Uh oh!

dkager commented Sep 23, 2018

Uh oh!

dkager left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 commented Oct 8, 2018

Uh oh!

lukaszgo1 commented Oct 18, 2018

Uh oh!

lukaszgo1 commented Dec 2, 2018

Uh oh!

Adriani90 commented Dec 3, 2018

Uh oh!

dpy013 commented Feb 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 commented Feb 15, 2019

Uh oh!

dpy013 commented Feb 16, 2019

Uh oh!

lukaszgo1 commented Aug 17, 2018 •

edited

Loading

lukaszgo1 Sep 29, 2018 •

edited

Loading