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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parent loggers to autocompletion #3345

Merged
merged 4 commits into from Mar 17, 2018

Conversation

4 participants
@daniel-beck
Member

daniel-beck commented Mar 12, 2018

screen shot

screen shot

Subsumes #3344 while at the same time doing a LOT more work 馃槅

Proposed changelog entries

Too minor.

Submitter checklist

  • [n/a] JIRA issue is well described
  • [n/a] Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • [n/a] Appropriate autotests or explanation to why this change has no tests
@Wadeck

Wadeck approved these changes Mar 13, 2018

馃悵 LGTM, I close mine.

@@ -87,14 +88,21 @@
@Restricted(NoExternalUse.class)
public AutoCompletionCandidates doAutoCompleteLoggerName(@QueryParameter String value) {
AutoCompletionCandidates candidates = new AutoCompletionCandidates();
List<String> candidateNames = new ArrayList<>();

This comment has been minimized.

@Wadeck

Wadeck Mar 13, 2018

Contributor

nit: could be a Set in order to optimize the candidateNames.contains(...) but as the list will not have lots of element (except for single letter search or "org", "com", etc.) the user will not see any difference.

This comment has been minimized.

@dwnusbaum

dwnusbaum Mar 13, 2018

Member

If it was a set you could remove the !candidateNames.contains(loggerNamePrefix) check entirely, but I agree that it shouldn't really make a difference.

@Wadeck

This comment has been minimized.

Contributor

Wadeck commented Mar 13, 2018

After some tests, it seems that the autocomplete does not provide consistency among user input but over server response time.

I mean that if I type org.random slowly, as org will return lots of results compared to org.random (empty normally), potentially the empty result will arrive at the client before the org and when it will receive the org result, it will display it, even if it's not the last thing the user typed.

To reproduce, put a breakpoint in LogRecorder#doAutoCompleteLoggerName's return like the following
autocomplete-over-input20
Then in the autocomplete field, type "hudson" then wait a bit, then add "z" (for hudsonz), you will see no suggestion as hudsonz does not exist but after a moment you receive the response for your initial request of "hudson".

PS: it's not related to your solution. I opened a ticket.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Mar 13, 2018

I really don't know whether this is a net improvement鈥

screen shot

@Wadeck

This comment has been minimized.

Contributor

Wadeck commented Mar 13, 2018

I really don't know whether this is a net improvement鈥

In terms of UX, I think the user is more confident if he wants to use "org" and see it in the list explicitely. So it's a +1 from me.

Concerning the order, I think the insertion order is not relevant since the LogManager.getLogManager().getLoggerNames() is not sorted.

screenshot_2018-03-13_163117_001

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Mar 13, 2018

@Wadeck Yes, I've learned that now as well. I'll investigate this when I'm next bored on the weekend :-)

daniel-beck added some commits Mar 15, 2018

Suggest all parent loggers that are relevant
Parent loggers will be suggested in auto completion iff they
include two or more child, grand-child, etc. loggers, and there
is no more specific parent logger that includes the same number
of descendants.
@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Mar 16, 2018

I spend a bit of time making this actually nice yesterday.

Current state:

screen shot

No loggers in org.jenkinsci.main that aren't in org.jenkinsci.main.modules, so don't suggest that.

@daniel-beck daniel-beck requested review from dwnusbaum and Wadeck Mar 16, 2018

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Mar 16, 2018

Major rework since previous reviews.

@Wadeck

Wadeck approved these changes Mar 16, 2018

Only small comment, nothing blocking.

No loggers in org.jenkinsci.main that aren't in org.jenkinsci.main.modules, so don't suggest that.

Nice UX improvement, good job 馃憤

  • thank you for the test, that avoid me to run the code with special case to be it works as expected :)
@@ -85,16 +87,60 @@
return ts;
}
@Restricted(NoExternalUse.class)
@VisibleForTesting
public static Set<String> getAutoCompletionCandidates(List<String> loggerNamesList) {

This comment has been minimized.

@Wadeck

Wadeck Mar 16, 2018

Contributor

As the order is kept, I think it could be nice to either explicit that in the name of the method or return a SortedSet

// Example: 'org' will show 'org', because there's org.apache, org.jenkinsci, etc.
// 'io' might only show 'io.jenkins.plugins' rather than 'io' if all loggers starting with 'io' start with 'io.jenkins.plugins'.
HashMap<String, Integer> seenPrefixes = new HashMap<>();
SortedSet<String> relevantPrefixes = new TreeSet<>();

This comment has been minimized.

@Wadeck

Wadeck Mar 16, 2018

Contributor

Normally I would have suggested to use new TreeSet<>(String.CASE_INSENSITIVE_ORDER), but the separation between the pure package name and the Logger name is somewhat nice. 馃憤

I hope we will never have package with uppercased name or class with lowercased name.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 17, 2018

Member

I hope we will never have package with uppercased name or class with lowercased name.

IIRC we have some such packages in the code, but I do not care much about such case. Performance is better with the standard ordering

}
// get names of all actual loggers known to Jenkins
Set<String> candidateNames = new LinkedHashSet<>(getAutoCompletionCandidates(Collections.list(LogManager.getLogManager().getLoggerNames())));

This comment has been minimized.

@Wadeck

Wadeck Mar 16, 2018

Contributor

perhaps split into two lines for readability ?

String longerPrefix = null;
for (int i = loggerNameParts.length; i > 0; i--) {
String loggerNamePrefix = StringUtils.join(Arrays.copyOf(loggerNameParts, i), ".");

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 17, 2018

Member

Sound like not the best implementation for high-depth classes

// Example: 'org' will show 'org', because there's org.apache, org.jenkinsci, etc.
// 'io' might only show 'io.jenkins.plugins' rather than 'io' if all loggers starting with 'io' start with 'io.jenkins.plugins'.
HashMap<String, Integer> seenPrefixes = new HashMap<>();
SortedSet<String> relevantPrefixes = new TreeSet<>();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 17, 2018

Member

I hope we will never have package with uppercased name or class with lowercased name.

IIRC we have some such packages in the code, but I do not care much about such case. Performance is better with the standard ordering

@daniel-beck daniel-beck merged commit 34b1e83 into jenkinsci:master Mar 17, 2018

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment