Previous terms in search dialog #8921
Conversation
There was a problem hiding this comment.
I should have chosen this radio button instead... Apart from user guide changes and making the source code comment a bit clearer (small word usage edits (exibit vs. shown, etc.) and copyright header changes), I'd say it is ready for the world. Test shows it is working as advertised, too. Thanks.
|
Hi, To @feerrenrut, can I request a second look, especially given the impact of changing the control used in this dialog? I'm also wondering if we can add a case for combo box layout (label + control) in GUI Helper so people defining combo boxes can use the same coding style as wx.Choice (addLabeledControl), and if yes, I'd like to suggest using find dialog as a test case. Thanks. |
|
Hi, By the way, I'm wondering if pop/insert calls can be replaced with in-place index swapping? One subtle optimization is to check the common cases first before resorting to looping like so:
As of now, NVDA will check the first item along with others. This means the following may happen:
Whereas I think if you search for the top item separately, NVDA won't have to loop through search history just to update the top item again. You don't have to think about optimizations at this stage because things can change based on feedback. Thanks. |
|
Responses inline
We can not because we are always privileging the current search term ratter than the previously listed one. Why? then we would throw the current search term and promote the listed one to the top of the list. This means that we are removing the old search term and inserting whatever the user typed at the beginning of the list. It can be the same term, but it can also be a term with different casing although it is the same term when compared in a case insensitive manner.
Implemented. Also made sure the whole thing does not execute when the current search term is blank
I don't kit understand why. This would arm code readability and increase the number of instructions to handle a very rare case. If we change the logic, we will spend some ifs / elses and may be save an enumerate call.
It all comes down to optimize versus improve readability / simplicity. As we have to take care of the casing and also are not on a critical execution path in terms of performance we could in my opinion privilege readability and simplicity instead of optimization.
P.S. I don't understand very well the code review interface here, so I don't know if I have to do something else than I did in this last commit. I refactored some of the comments in the code and implemented the changes you asked for, at least the changes I was able to find. Bear in mind that this is my very first contribution, so if something else needs to be adjusted please let me know and I will do it. What should I type in the copyright file? Thanks, |
|
Hi, as for optimizations, let’s not do it for now. Regarding changelog, I’ll let someone from NV Access do it, so no need to touch changes.t2t. As for copyright data, I’d say don’t worry – I am about to go through 2018.4 commits and changelogs to see if some small edits can be done anyway, so I’ll take care of copyright header for you. As for other changes, no, I don’t think so. Thanks (very impressive first feature pull request).
From: Marlon Brandão de Sousa <notifications@github.com>
Sent: Tuesday, November 6, 2018 6:06 PM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Comment <comment@noreply.github.com>
Subject: Re: [nvaccess/nvda] Previous terms in search dialog (#8921)
Responses inline
Hi,
By the way, I'm wondering if pop/insert calls can be replaced with in-place index swapping?
We can not because we are always privileging the current search term ratter than the previously listed one.
Why?
Because of this: If the user first searched for "Car" (c uper case) and then by
"car" (c lowercase), we want the term "car" (c lower case) to be kept in the search list instead of the original "Car" (c uper case) as we can not keep both.
If the code were:
searchEntries.pop(index)
searchEntries.insert(0, item)
then we would throw the current search term and promote the listed one to the top of the list.
But notice that the code is:
searchEntries.pop(index)
searchEntries.insert(0, currentSearchTerm)
This means that we are removing the old search term and inserting whatever the user typed at the beginning of the list. It can be the same term, but it can also be a term with different casing although it is the same term when compared in a case insensitive manner.
Therefore, we are solving a problem in an elegant way at a very small cost, otherwise we would have to create yet another if / else to verify if the terms are exactly the same so we perform index swapping or if they differ in casing so that we use the same solution we are using.
One subtle optimization is to check the common cases first before resorting to looping like so:
1. If search history is empty, just add whatever the user says.
Implemented. Also made sure the whole thing does not execute when the current search term is blank
2. Check the most recent item separately.
I don't kit understand why. This would arm code readability and increase the number of instructions to handle a very rare case.
Usually, a user will retype an already listed term if:
1- It is far down the list and the user does not want to spend time searching for it, preferring to type it again.
2- The user has forgotten that the term is already in list, which is unlikely to happen if it is the last searched item.
3- but even if this is the case, for example if the user makes a nvda + ctrl + f and presses enter to search again for the current term (instead of pressing nvda + f3 ... ), we will enumerate the items and start the loop. The very first iteration at item 0 would point that the current item is equals to the item 0. We will swap (we need to do so because the user might have typed the same term but with different casing as explained above) but soon after we have a return statement, preventing the loop from going on. So we won't have a loop, we will have one iteration only.
If we change the logic, we will spend some ifs / elses and may be save an enumerate call.
3. Only then loop through.
As of now, NVDA will check the first item along with others. This means the following may happen:
1. NVDA starts, user types something into the find entry field, and that gets added to an empty search history.
2. User searches for more terms, and NVDA will check each history item to see which one should be bumped to the top.
3. User enters the most recently searched item again. NVDA will loop through once more, and seeing that the item entered is the most recently searched one (index 0), it'll pop index 0, and then insert the searched item at the top of the search history.
Whereas I think if you search for the top item separately, NVDA won't have to loop through search history just to update the top item again. You don't have to think about optimizations at this stage because things can change based on feedback.
It all comes down to optimize versus improve readability / simplicity. As we have to take care of the casing and also are not on a critical execution path in terms of performance we could in my opinion privilege readability and simplicity instead of optimization.
Thanks.
P.S. I don't understand very well the code review interface here, so I don't know if I have to do something else than I did in this last commit. I refactored some of the comments in the code and implemented the changes you asked for, at least the changes I was able to find. Bear in mind that this is my very first contribution, so if something else needs to be adjusted please let me know and I will do it.
What should I type in the copyright file?
How do I edit the changelog? Is it something I have to do in git or is it something the CI will pull from this PR template?
Thanks,
Marlon
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#8921 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkGFvB9swJbZ9J2e6gwwqEjk9gUuPks5uskAZgaJpZM4YPnho> .
|
|
Hi, Congratulations; at least the reviewer (my) part is done. Let's wait for lead developers to merge it into master, and then I'll take care of user guide and copyright header. Thanks. |
…/nvda into previousTermsInSearchDialog Updating branch to last NVAccess merge state
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
As I pointed out in #8921 (comment), while I understand the concerns, this is not the place to discuss them. Please use the development mailing list for this and refrain from discussing them on github. Update: I've marked the comments above as offtopic. Again, how valid some concerns may be, discussing them on github diverts the attention from the development and review process. |
|
I've had a quick discussion with @feerrenrut about this pr. Before he has got the time to look into this, it might help to answer the following questions:
|
|
Hello Leonard, We have considered a permanent search history. However, this would add complexity in terms of keeping the history in an encrypted form some hwere in the NVDA profiles system. This would need to have a way on NVDA settings also to clear the history. Now, the use cases of myself and most part of commenters on the related issues seem not to consider the permanent history an essential feature, because you will most likely use previous terms in an repeated flow where you look first for one thing and then for a second one but latter on you need to repeat that standard, looking for the same first thing and then for the second one. If you look at the implementation, we are not even keeping the history among browsers. Now, I believe in incremental updates to features. This is one thing I have learnt after so many years building systems. You build fast a basic version of a feature. This version will help people and add value earlier to those who need it. You then monitor and collect feedbacks. If an evolution of the feature is needed / wanted, you estimate it and code it. If not, (e.e users are happy with the way things stand) then you focus your energy om other things. About limiting the amount of terms, this can surely be done. See, putting this functionality on the pr will change one class, two, three lines may be at maximum. Implementing a permanent history would add many more lines in many different places. So I don't think we should over engineer this functionality. Instead, I would merge it as soon as possible, since it has been available already for two versions of NVDA and it could have added value for users now for at least three, four months. We could discuss next steps once we have collected feedbacks. What do you think? |
I have checked that. Here is what happens: wx.SearchCtrl is a self contained very simple search dialog. |
|
Other requested changes have been submitted as requested. |
source/cursorManager.py
Outdated
| @@ -27,6 +26,10 @@ | |||
| import ui | |||
| from textInfos import DocumentWithPageTurns | |||
|
|
|||
| # search history list constants | |||
| SEARCH_HISTORY_FRONT = 0 | |||
| SEARCH_HISTORY_MAX_SIZE=20 | |||
There was a problem hiding this comment.
I think it would be much clearer to use SEARCH_HISTORY_FIRST_INDEX = 0 and SEARCH_HISTORY_LAST_INDEX = 19. From this it's obvious there are 20 elements, there is no conversion required between size and final index eg: del entries[(SEARCH_HISTORY_MAX_SIZE -1):], it's clear we are talking about indices rather than values.
While in C++ it's a common pattern to define the size as a constant, this is typically because you want to pre-allocate your data structure. This is not necessary here, and in fact SEARCH_HISTORY_MAX_SIZE is never used directly at all.
There was a problem hiding this comment.
I think it would be much clearer to use
SEARCH_HISTORY_FIRST_INDEX = 0andSEARCH_HISTORY_LAST_INDEX = 19. From this it's obvious there are 20 elements, there is no conversion required between size and final index eg:del entries[(SEARCH_HISTORY_MAX_SIZE -1):], it's clear we are talking about indices rather than values.
This is a subjective matter. I have chosen to use constants naming the "business" representation ratter than the computational representation.
From an algorithm point of view, I am inserting items at the front of the list and I am checking if the length of my list is greater than the maximum amount of items allowed for that list, and this is why the SEARCH_HISTORY_MAX_SIZE exists. May be I should have used SEARCH_HISTORY_MAX_LENGTH instead, in Portuguese these two words translate to the same word and this is why I had that mistake..
If the list is now greater or equals than the maximum length of items, 20 items (not 19) then I truncate it to 19 items. And I do so because right at the next line I will insert the 20th item at the front of the list.
In fact, the only function having to deal with indexes, which play the computation not the business roles, is _truncateSearchHistory. Because it has to deal with computational requirements, such as converting a length one wants to truncate the search history to a index needed by python, its logic has been separated and encapsulated on a private method. The rule is that, at the moment, it is necessary to convert a length (business realm) to an index (computation realm) by subtracting one from the desired length because python is zero indexing collections. If the maximum length changes, the corresponding index must be equals to the new length minus one.
While in C++ it's a common pattern to define the size as a constant, this is typically because you want to pre-allocate your data structure. This is not necessary here, and in fact
SEARCH_HISTORY_MAX_SIZEis never used directly at all.
It is directly used in line 88:
if len(searchEntries) >= SEARCH_HISTORY_MAX_SIZE:
Notice that except by the word size instead of length this is basically raw English: if length of searchEntris is greater than or equals to the maximum allowed length for this list then ...
truncate the list so that it is the size it should be minus one ... and then
add the current term to the front of the list
So I would define the constants exactly the way they are. However, you are the reviewer, not me and, to be franc, users will benefit from the search history, not from this little discussion. although I myself appreciate it because it is always good to talk to and have code reviewed by talented programmers.
I will therefore implement the suggested changes and commit them possibly in one day.
wx.SearchCtrl does not seen to offer. Checkout the SearchCtrl demo code, it allows for any number of extra options. I think this would meet our needs well. We can take this without support for case sensitive history, though considering we have the option for case sensitive comparisons, it's strange not to save case sensitive history. |
|
Also, can you update the user guide, and update the PR description with a change log entry. |
I can try. I have never done so and I don't know how to do it, nor have I found documentation about this topic. What should go into the users guide? The dialog operation didn't change. Perhaps a note on the what is new section, this would be certainly worth ... but is there any specific order where it should go? What about the changelog? What do you mean by update the pr description to generate an entry in the changelog? Do I have to put a given sign so a parser of some type will read it and auto generate the file or what should I do? Again, thank you very much for reviewing and helping to add value to users by putting this in a state where it can be merged ... |
I expect there is a section of the user guide that mentions the find dialog. If not, please add one. This section should be updated to mention that the find dialog holds a history of recently searched items, I don't think there is a need to mention the number of items. I would also mention that it is expected that this will not hold items that differ only by case.
Essentially, you need to describe the changes required in the whats changed / change log document. The pull request template talks about this. Look at the file These entries are essentially in the format: |
|
Hi, for user guide, I think adding a sentence or two to find dialog section explaining that search history is available would suffice. If you need help, let me know. Thanks.
|
|
User guide has been updated. Please @josephsl go ahead and update it again if you you think something more is needed. As a last thing, we are storing capitalized search terms. What happens is that if user searches for ball and then for Ball (b capitalized) the ball (b not capitalized) will be removed from the list and Ball (b capitalized) will be kept. The most current search term will always be preferred if there are terms conflicting in capital letters. |
Added comments to explain the how search entries are stored. Removed unnecessary max items constant, last index can be used instead. Replaced magic numbers referring to most recent item. Changed names of the constants to give better hints about ordering.
Added more information to the userguide. Ensure each sentence is on a newline, and remove double spacing.
|
@marlon-sousa I have committed a couple of changes directly to this branch. In particular, for future PR's ensure that all sentences in the user guide are on their own line. This helps with translation. Normally we don't make these kinds of code review changes directly, but to try to move this change along and prevent it stagnating I though this would be more pragmatic. Can you please re-test this branch? |
|
I am sorry. I have used notepad plus plus and as far as I can remember sentenses were, at least for me, each on their corresponding line. I will try to figure out what went wrong. I will be testing it again in some hours.
As far as I am conserned you are free to commit stuff .. the important thing is to deliver value to the users as fast as possible. Thank you again for reviewing and helping.
Obrigado,
Marlon
Em 20 de mar de 2019, à(s) 13:56, Reef Turner <notifications@github.com> escreveu:
… @marlon-sousa I have committed a couple of changes directly to this branch. In particular, for future PR's ensure that all sentences in the user guide are on their own line. This helps with translation.
Normally we don't make these kinds of code review changes directly, but to try to move this change along and prevent it stagnating I though this would be more pragmatic. Can you please re-test this branch?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@feerrenrut I have corrected a word on the user guide and pushed a new commit. Tests were successful so for me it is ok. I also report that making a git diff @~..@ showed me the adjusts you have made to the text itself, but I couldn't see any differences regarding line breaks in the user guide. |
|
Thank you for this PR! In section 6.3 of the User Guide, it says to use NVDA+f for this search dialogue. But this should be NVDA+control+f. |
|
Thanks @dan1982code. This should be fixed in the next alpha build. (commit 00a63f0) |
|
Hi all, Based on IME regression notes from @nishimotz, I'd like to propose reverting this, as it introduces issues for several language communities. Thanks. |
Although useful, search history combo box does not allow languages requiring IME (input method editor) to cancel entry of search text. This affects Chinese, Japanese, Korean and other languages. This reverts commit e3b2e79.
Revert "History in search dialog (PR #8921)" Although useful, search history combo box does not allow languages requiring IME (input method editor) to cancel entry of search text. This affects Chinese, Japanese, Korean and other languages. This reverts commit e3b2e79. Keep the "Searching for text section" in the user guide as most of the section is accurate and there are links to this section in other parts of the document. Only remove lines that refer to search history.
Link to issue number:
Closes #8482
Also see #8915
Summary of the issue:
Display a list of previously searched terms in the find dialog used with cursor manager
Description of how this pull request fixes the issue:
We replaced the textCTRL with a WXCommbobox in the FindDialog class used by CursorManager to show the find dialog.
The constructor of FindDialog receives a reference to the list of previously searched terms and feeds the WXComboBox with this list.
The onOk method, activated when the user performs a search, gets the new search term and updates the list of previously searched terms before performing the search.
The list update process is performed by the new updateSearchEntries method.
This method:
1- Verifies if the current searched term is already listed.
1.1- If it already is in the list, promotes the current searched term to the first place ** or beginning ** of the list, so that it will appear selected by default when the find dialog is activated again, just like the last searched term is filled in the current implementation.
1.2- If it still is not in the list, push it to te beginning of the list.
The list of previous searches is created empty as a property in the CursorManager class.
It is passed as a parameter to the FindDialog constructor in the script_find method.
Testing performed:
1- Open a browser window.
2- Opem www.google.com
3- Activate find dialog.
4- a Blank edit combo must appear.
5- Type mail and press enter.
6- The gmail link is found.
7- Ctrl + home.
8- Activate find dialog.
9- A edit combo with one value, "mail", must appear.
10- Type mag and press enter.
11- The images link is found.
12- Activate find dialog.
13- A edit combo with the values "mag, mail" must appear. The value "mag" must be selected by default and appear on top of the list.
14- Select the "mail" value and press enter.
15- Activate again the find dialog. An edit combo with the values "mail, mag" must appear. The "mail" value should appear selected by default and must be on top of the list.
16- The behavior must be the same doesn't matter if the terms are lower, upper or mixed case. No duplicates must appear on the list and the last searched term, regardless of being or not previously on the list, must always appear selected by default and in the beginning of the list in the next find dialog exhibition.
Known issues with pull request:
WXCombobox can't present items that only differ in case on MS Windows. This means that two versions of a given term which differ only in case such as "Car" and "car" can not coexist. We have taken the decision of storing the last searched term in this list, so if one has searched first for "Car" and then by "car" the list will retain only the "car" value in subsequent find dialog exhibitions.
Change log entry:
Section: New features