Fix empty page tree in pre-filtered preference dialog#4044
Conversation
|
@wahlbrink FYI, I also tested your example and it works fine. |
There was a problem hiding this comment.
With this change, the filterered trees in preference dialogs don't have any hint on the filter field at all anymore. Is that intended?
For the filtered preference dialog one might argue that it's okay because the filter text was only presented as ordinary filter text input when opening the dialog, so it might be somehow okay to remove that:
But the ordinary preferences dialog now looks like this:
While it used to contain a proper hint:
Edit: just saw that there is the search symbol in front of the text now. Still I thought that there is a possibility to show a hint in such a search box, isn't it? I would rather expect that to be shown (like it was in the preferences dialog before). Otherwise it becomes less obvious that this is filter field rather than a search field.
|
The search icon is the hint. I can add the text back if you prefer. |
|
In the current As said, in my opinion only having the search icon is wrong, as this is not a search field but a filter field. One may even argue that the search icon is not correct as it would need to be a filter icon. |
vogella
force-pushed
the
fix-4041-prefilter-dialog
branch
2 times, most recently
from
5b5ddc5 to
be72249
Compare
|
I restored the text hint |
The native search field rework (PR eclipse-platform#3925) overrode setInitialText to only set the placeholder message and stopped maintaining the inherited initialText sentinel. The pre-filter path addFilter() still wrote the hint "type filter text" into the field, which was then applied as a real filter pattern (since it no longer equalled initialText), matching no preference pages and leaving the tree empty. Keep the initialText sentinel in sync in the override and stop writing the hint into the field in addFilter(). The placeholder hint is still shown via setMessage so the field keeps signalling that it filters the pages, but it is never written into the field as an active pattern. Fixes eclipse-platform#4041
vogella
force-pushed
the
fix-4041-prefilter-dialog
branch
from
be72249 to
e783f80
Compare
|
Is there a specific reason why my review comment was left uncommented and the PR was merged without responding to it? |
|
Due to lack of response to my review comment, merging the PR despite the open review comment and ignoring my comment left after merging the PR, I plan to revert this change. @vogella fyi |
I thought I did adjust the pr based on your request. Is it now in sync with the master in the release, was that not your request @HeikoKlare ? |
I assumed that was your suggestion. |
|
As said, my review comment is completely unanswered: #4044 (comment) |
I restored the message based on your request, hence this feedback was outdated as we do not remove this text. Technical we have this: The message is not obsolete. FilteredTree_FilterMessage is still used at FilteredTree.java:294 and the comparison at line 619 still applies to the base FilteredTree and the subclasses that write the message into the field content. In FilteredPreferenceDialog we now keep the field empty instead of writing the message into it, but narrowingDown evaluates identically: with previousFilterText empty, getFilterString().startsWith("") is always true, which is the same result the equals(FilteredTree_FilterMessage) branch produced before. So no usage becomes dead and there is no behavioral change in that path. |
|
I think you said two things in your feedback: "We should not remove the text" and "If we remove the text, this NLS becomes obsolete" so I assumed "I restored the text" answered both, I should have been more explicit about it, to avoid misunderstanding. |
That's not correct. You restored the message, but not the text. The question I asked refers to the text being compared. So with this change the filtered preference dialog behaves different in terms of UX for filtered trees than all other filtered trees (they still provide the message as initial text, which is why the code I asked for probably needs to stay). Is that intended? Don't we want to have consistent UX thoughout the usages of filtered trees? |
|
On the inline question: the FilteredTree_FilterMessage NLS isn't obsolete. It's still used at FilteredTree.java:294 and the comparison at line 619 still applies to the base FilteredTree and subclasses that write the message into the field value. In FilteredPreferenceDialog we now keep the field empty, but narrowingDown evaluates identically (with an empty previousFilterText, getFilterString().startsWith("") is always true, matching the old equals(FilterMessage) branch). So nothing becomes dead and there's no behavioral change. On the hint itself: the intention of #3925 was to move the filter field away from the initial-text-in-value pattern, which is a recognized UX anti-pattern (e.g. https://www.nngroup.com/articles/form-design-placeholders/): screen readers can treat it as a real value, and it needs clear-on-focus handling. setMessage gives the same visible hint without those problems, and the hint is now restored that way. I agree we should be consistent. My preference is to move the remaining places that still push the hint into the field value over to setMessage too, rather than reintroducing the old behavior here. Changing the global default is harder to land in this project, so I'd treat that separately. That said, if you feel strongly that the old initial-text behavior should be restored, I'm happy to do it. Let me know. |
No, I do not want to propose to move back. I fully agree that properly embedding the hint via |
I agree. I will create a PR for that and add you as reviewer to have the discussion there. |
|
FYI - AbstractFilteredViewerComposite is the place to do this, not FilteredTree, will be visible in the upcoming PR. |
2 participants
Forward fix for #4041 on master (the 4.40 maintenance branch gets the plain revert in #4043).
The native search field rework in #3925 stopped maintaining the inherited initialText sentinel, so the hint that addFilter() wrote into the field was applied as a real filter pattern and hid every preference page. This keeps the sentinel in sync and no longer writes the hint into the field, so a pre-filtered dialog (for example Preferences from a Java editor context menu) shows its pages again. The native search field is preserved, and the "type filter text" hint is kept as the placeholder message so the field still signals that it filters the pages.