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 filtering functionality to the symbols list #8790

Merged
merged 17 commits into from Apr 23, 2019

Conversation

@leonardder
Copy link
Collaborator

@leonardder leonardder commented Sep 27, 2018

Link to issue number:

Closes #5761

Summary of the issue:

Now that emoji's have been introduced in NVDA symbol dictionaries, the list of symbols was getting pretty big. This also introduced a major lag when opening the speech symbols dialog.

Description of how this pull request fixes the issue:

  1. Added a filter edit box to the speech symbols dialog. @feerrenrut: your opinion about the visuals of this edit box are greatly appreciated. Feel free to push directly to this branch if you need to change things visually.
  2. Converted the list control to a list control with the wx.LC_VIRTUAL style. I'm really exited about the major performance boost this gives when loading the dialog for the first time, as well as when filtering. It is just really fast now!
  3. Added some functionality to gui.nvdaControls.AutoWidthColumnListCtrl to support getting item text from a data storage without the requirement of subclassing the list control. It is now also possible to forcefully send a wx.EVT_LIST_ITEM_FOCUSED to the control

Testing performed:

Filtering, adding and removing items, closing the dialog and reopening it again quickly after that.

Known issues with pull request:

  • The visuals might not be ideal yet, though guiHelper is really amazing in creating stuf that just looks good.
  • We should probably wait for #8006 to be in master and apply scaling support to this dialog as well while at it.
  • This does not yet apply to speech dictionaries. I think it should at some point, but that won't be part of this pr.

Change log entry:

  • New features

    • You can now filter symbols in the punctuation/symbol pronunciation dialog, similar to how filtering works in the elements list and input gestures dialog. (#5761)
  • Bug fixes

    • Loading the punctuation/symbol pronunciation dialog is now much faster when using symbol dictionaries containing over 1000 entries.
@dnz3d4c
Copy link

@dnz3d4c dnz3d4c commented Sep 27, 2018

Korean symbol dictionary has many items. Can you expect to see speed improvements when navigating the list box with this patch?

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Sep 27, 2018

Wow! I'm coming across like 31000 symbols here, taking like 20 seconds to load with the current Alpha version of NVDA. This pr reduces that to 2 seconds.

@dnz3d4c
Copy link

@dnz3d4c dnz3d4c commented Sep 27, 2018

Thanks.

Wow! I'm coming across like 31000 symbols here, taking like 20 seconds to load with the current Alpha version of NVDA. This pr reduces that to 2 seconds.

@xingkong0113
Copy link
Contributor

@xingkong0113 xingkong0113 commented Sep 27, 2018

@hi leonardder I am the maintainer of NVDA Simplified Chinese. I found that NVDA can't generate Simplified Chinese emoticon dictionary using the scons command.

@josephsl
Copy link
Collaborator

@josephsl josephsl commented Sep 27, 2018

@xingkong0113
Copy link
Contributor

@xingkong0113 xingkong0113 commented Sep 28, 2018

thank josephsl

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Sep 28, 2018

I must have missed the Chinese annotations.

@dingpengyu: Could you please open a new issue for this? I'm not yet known with the various differences in the Chinese language, so it is quite difficult to decided what would be the best assignments. In NVDA, we have the zh-cn, zh-hk and zh-tw locales.

@josephsl
Copy link
Collaborator

@josephsl josephsl commented Sep 28, 2018

@xingkong0113

This comment was marked as off-topic.

@xingkong0113
Copy link
Contributor

@xingkong0113 xingkong0113 commented Sep 28, 2018

Hi, thanks to josephsl's reply, this is the issue address:
NVDA cannot generate Simplified Chinese emoticon dictionary using the scons command · Issue #8799
#8799

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Dec 6, 2018

@feerrenrut: I've removed the blocked label since #8006 is merged. However, I'm not yet sure what to do to make this dialog scale correctly, other than setting resizable to True. The size of the list control and its columns is undefined.

Copy link
Member

@feerrenrut feerrenrut left a comment

When filtering, the list does not update. The entries only update as they gain focus. Visually this is quite confusing.

Because this dialog is a SettingsDialog Then there is no need to do anything to get the DPI scaling, except, if you specify a size manually, you must pass it through self.scaleSize().

I think this dialog could be wider, because the text in the replacement column is often long enough to exceed the width of the column. The replacement field (once populated) often isn't wide enough. There is plenty of space to widen this field, and the dialog itself isn't very wide either.

Some emoji don't seem to be supported by the font. Visually they are just squares. Not a blocking issue right now, since I believe this is already the case. An issue should be created for this.

I can take a look at these points.

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Jan 18, 2019

When filtering, the list does not update. The entries only update as they gain focus. Visually this is quite confusing.

Ugh, might this be because the list is virtual? Does this also mean that there's only one item visible at a time or something like that?

source/gui/nvdaControls.py Outdated Show resolved Hide resolved
@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Jan 18, 2019

Does this also mean that there's only one item visible at a time or something like that?

No, the list looks correct. After entering a filter the list does not seem to change, until you move focus to the first item, and it suddenly changes. Moving to the next item causes that one to change.

feerrenrut and others added 4 commits Jan 18, 2019
Otherwise the items only change when they receive focus. It appears (visually)
as if the list has not changed, until you start to scroll or select items.
…em text at construction time of an AutoWidthColumnListCtrl.

A weak reference to the callable is saved on the control. If we wouldn't, closing the punctuation dialog and reopening it would result in a multi instance error.
The extensionPoints module provides functionality to deal with instance method weakrefs. Updated that code to allow weak references without an unregister callback.
@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Jan 19, 2019

@feerrenrut: Thanks for fixing the list updating issue. Would be great if you can fix the replacement column width as well, since it would really help to do that with a pair of eyes.

@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Jan 21, 2019

Yep, I'll take a look at the widths in the dialog tomorrow.

Copy link
Member

@feerrenrut feerrenrut left a comment

I've looked through this again and I'm happy for it to be merged. @leonardder Are you happy with the latest changes?

Copy link
Collaborator Author

@leonardder leonardder left a comment

I've got some comments. Also, you could consider adding a convenience wrapper for the new LabeledTextControl to BoxSizerHelper.

source/gui/guiHelper.py Outdated Show resolved Hide resolved
source/gui/guiHelper.py Outdated Show resolved Hide resolved
source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
@xingkong0113
Copy link
Contributor

@xingkong0113 xingkong0113 commented Apr 4, 2019

hi
Is there a developer review for this PR? Seeing that it is already approved

feerrenrut added 3 commits Apr 9, 2019
This was a clumsy attempt to be able to support making the textCtrls resize
with the dialog. The real problem is that GuiHelper is not felixible enough
to do it in a simple way. For now, we will just set a reasonable starting
size.
The parameter was deceptively named "index" where it affected columns
numbers, first column is 1 not 0.
Reduce nesting, clarify some comments.
@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Apr 11, 2019

@leonardder I've made a few changes here, if you are happy with this I think it is ready to be merged.

Leonard de Ruijter added 2 commits Apr 13, 2019
…y to focus a nonexistent item. Also properly disable the remove button when disabling the controls
@leonardder leonardder requested a review from feerrenrut Apr 13, 2019
@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Apr 13, 2019

@feerrenrut: I'm ok with your changes and did a small round of tests.

Found and fixed the following:

  1. Add a new symbol, for example called test
  2. Filter on test
  3. Remove the test symbol

What happened is that the list of symbols was empty, yet the old logic tried to select a new symbol. Removing a symbol didn't check for the item count properly. Now it does.

Copy link
Member

@feerrenrut feerrenrut left a comment

Looks good. I'll merge this in.

@feerrenrut feerrenrut merged commit 07e48a9 into nvaccess:master Apr 23, 2019
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2019.2 milestone Apr 23, 2019
feerrenrut added a commit that referenced this issue Apr 23, 2019
New features:
- You can now filter symbols in the punctuation/symbol pronunciation dialog, similar to how filtering works in the elements list and input gestures dialog. (#5761)
Bug fixes:
- Loading the punctuation/symbol pronunciation dialog is now much faster when using symbol dictionaries containing over 1000 entries. (#8790)
feerrenrut added a commit that referenced this issue Sep 16, 2019
The replacement control was added to the wrong sizer.
Issues: (#8790, #10216)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants