Foobar2000: implement a custom status bar routine to fix playback status commands by josephsl · Pull Request #11087 · nvaccess/nvda

josephsl · 2020-05-01T23:19:10Z

Hi,

This is a follow-up to #9792:

Link to issue number:

Summary of the issue:

In Foobar2000 with NVDA alpha.20080 and later (with custom status bar commit), NVDA says 'no track playing" when trying to obtain elpased time, total time, and remaining time for a playing track.

Description of how this pull request fixes the issue:

Implemented a custom status bar fetcher for Foobar2000 that uses windowUtils.findDescendentWindow to retrieve the status bar for the foreground object.

Testing performed:

Tested with Foobar2000 with latest alpha and with app module changes applied through this PR.

Known issues with pull request:

None

Change log entry:

None, as it is a follow-up to a change in alpha

… errors when obtaining playing duration. Re nvaccess#11082. NVDA now lets app module classes define customer retriever for status bars. However, Foobar2000 uses 'statusBar' as a variable name, causing status bar retriever to fail. This leads to NVDA announcing 'no track is playing' when trying to obtain track information (elapsed time, for example). This is now resolved by defining Foobar2000 specific status bar retriever, utilizing windowUtils.findDescendantWindow and returning the resulting status bar object if one is found.

JulienCochuyt · 2020-05-02T05:09:44Z

All apologies for missing this impact.

@josephsl,
Thanks for taking care of cleaning my mess.
While renaming the statusBar attribute in appModules.fooBar2000.AppModule to something like statusBarObj would have been enough to fix #11082, I understand how your approach is more efficient than the default api.getStatusBar.
Still, I would advocate for the following changes to this PR:

Add a comment clarifying that the technique used is not a replacement for the failing standard one, but rather an optimization.
Fallback to raising NotImplementedError rather than returning None: This way, if a foobar2000 plug-in displays a status bar which does not have the class name "ATL:msctls_statusbar32", its content could still be read. Or do you know for sure such a scenario is unlikely to happen? (I'm only a seldom user of foobar2000, and unaware of the diversity of its plug-ins.)

josephsl · 2020-05-02T05:26:10Z

HI, no need to apologize over it (I made the same mistake several times in my own code). I think NotImplementedError makes sense if _get_statusBar isn’t defined in the first place. Since it is encountered inside the very thing it is supposed to do, it could amount to lying at the source code level. As for that check, as I’m not a seasoned Foobar2000 user myself, I’d ask users.

lukaszgo1

I'm slightly worried about performance here. Previously the status bar object was fetched once and then stored as an attribute of the appModule instance, whereas now it is fetched each time when one of the time requesting scripts is executed

lukaszgo1 · 2020-05-02T12:23:36Z

-# Copyright (C) 2009-2018 NV Access Limited, Aleksey Sadovoy, James Teh, Joseph Lee, Tuukka Ojala
+# Copyright (C) 2009-2020 NV Access Limited, Aleksey Sadovoy, James Teh, Joseph Lee, Tuukka Ojala
 # 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.htmlimport appModuleHandler


Could you please clean-up this line while at it?

josephsl · 2020-05-02T18:10:07Z

Hi,

Copyright header: I need cleanup reasons, as all I did there is update the copyright header (we will need to go through files again to do copyright header management at some point).

As for performance and previous implementation: it was done that way for consistency with what Julian wrote.

There is an issue that was present in old and current implementations that will require a separate PR to fix: NVDA will get wrong status bar if something other than the playlist gets focused when NVDA starts. This is caused by gain focus event caching the "active" status bar from anywhere as long as status bar isn't defined. The ideal solution is either using Foobar2000 API to announce playback time, or locating the status bar object from Foobar2000's main window handle. Resolving that issue means there is no need for this PR, or better yet, modifying the PR to account for changes introduced in recent alpha snapshots along with a better solution for #11082.

Thanks.

lukaszgo1 · 2020-05-02T18:25:50Z

The line says:
# For more details see: https://www.gnu.org/licenses/gpl-2.0.htmlimport appModuleHandler

I don't think import appModuleHandler should be there :-)

josephsl · 2020-05-02T18:35:10Z

Hi, you’re right – thanks for catching this (Tuukka Ojala was the last person who edited this app module file).

josephsl · 2020-05-04T19:07:50Z

Hi, part of that has to do with recursion limit – I’ll add a more friendly explanation of what I just said. Thanks.

… bar getter is defined. Re nvaccess#11082.

AppVeyorBot · 2020-05-04T20:25:35Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit abd6889864

feerrenrut · 2020-05-07T07:44:51Z


 	def _get_statusBar(self):
 		# #11082: retrieve status bar handle and resulting NVDA object from playlist window.
+		# If only default status bar routine is used to assign status bar object,


Do you mean that calling api.getStatusBar() from here leads to a recursion error? That's true, and was a concern when reviewing #9792. In the end I decided that any future work could split that function to break the recursion.

This is really a tangent to what I was trying to ask. The way that this works is different from api.getStatusBar(), is there a reason? In api.getStatusBar() we hit test 1 pixel up from the bottom of the foreground object and walk up the tree looking for the statusbar role. This implementation enumerates all child windows of the ForegroundObject looking for this particular class name.

josephsl · 2020-05-07T07:59:43Z

Hi, on-screen method could have worked here. My concern was about attribute name and to better detect status bar from main window. Thanks.

feerrenrut · 2020-05-08T14:25:55Z

and to better detect status bar from main window

But can you explain why this approach is better?

josephsl · 2020-05-08T17:32:15Z

Hi,

The biggest advantage is looking at status bar that is located somewhere other than bottom left. So far, Foobar2000 does not exhibit this (I know several apps such as GoldWave that exhibit this case).

After thinking about this, I think it would be best to go with renaming the attribute (option 1) due to 2020.2 beta preparation. I'll submit a different PR that should resolve this (and an advice for future committers: when adding or changing class attributes, be sure to check everywhere in the source code (or relevant sections) to make sure this kind of regression is minimized).

Thanks.

josephsl added 3 commits May 1, 2020 16:07

appModules/foobar2000: update copyright year

ab4c34f

appModules/foobar2000: correct issue number in comment.

80d8fdd

josephsl mentioned this pull request May 2, 2020

AppModule: Customize retrieval of status bar object or TextInfo (#4640) #9792

Merged

lukaszgo1 reviewed May 2, 2020

View reviewed changes

appModules/foobar2000: fix copyright header formatting issues.

9a0eada

feerrenrut suggested changes May 4, 2020

View reviewed changes

Comment thread source/appModules/foobar2000.py

appModules/foobar2000: add an explanation regarding why custom status…

14224b3

… bar getter is defined. Re nvaccess#11082.

appModules/foobar2000: lint (split long comment)

d6fa28f

feerrenrut reviewed May 7, 2020

View reviewed changes

josephsl closed this May 8, 2020

josephsl deleted the i11082foobar2000StatusBar branch May 8, 2020 17:35

Uh oh!

Conversation

josephsl commented May 1, 2020

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!

JulienCochuyt commented May 2, 2020

Uh oh!

josephsl commented May 2, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszgo1 left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 May 2, 2020

Choose a reason for hiding this comment

Uh oh!

josephsl commented May 2, 2020

Uh oh!

lukaszgo1 commented May 2, 2020

Uh oh!

josephsl commented May 2, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

josephsl commented May 4, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AppVeyorBot commented May 4, 2020

Uh oh!

feerrenrut May 7, 2020

Choose a reason for hiding this comment

Uh oh!

josephsl commented May 7, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented May 8, 2020

Uh oh!

josephsl commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

josephsl commented May 2, 2020 via email •

edited by feerrenrut

Loading

josephsl commented May 2, 2020 via email •

edited by feerrenrut

Loading

josephsl commented May 4, 2020 via email •

edited by feerrenrut

Loading

josephsl commented May 7, 2020 via email •

edited by feerrenrut

Loading

josephsl commented May 8, 2020 •

edited

Loading