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

[WIP] Refactor auto completion #1077

Closed
wants to merge 7 commits into from
Closed

[WIP] Refactor auto completion #1077

wants to merge 7 commits into from

Conversation

stefan-kolb
Copy link
Member

Major changes in this PR:

  • Only use auto-completion when at the end of a word (space after)
  • Detection modes: Only use case-sensitive detection for now (performance, logic, we can re-add functionality from here)
  • Remove method getPrefix() - Intention of this method was unknown
  • Use Nameautocompleter for every PERSONNAME BibTeX field, not only author & editor

@stefan-kolb
Copy link
Member Author

The current auto completion has two modes:

  • a case sensitive mode
  • a case insensitive mode

The case sensitive mode is activated when one types a word that has mixed case, e.g., gZhhUh.
If the word is only lowercase the auto completion will perform a case insensitive mode.

The case sensitive mode is straight-forward: It will lookup a word that can be completed from the current partial word.
Case insensitive lookup tries to find a word that can be completed from the input based on ignore case search. If it found one, it will still present the word in its original casing, e.g., some -> Something

Discussion point
What is the purpose of the case-insensitive mode? The detection (only lowercase in word) seems arbitrary to me. How should the completion be performed in general or what are typical algorithms for this? I would either suggest case-sensitive or a possibility to configure this but no auto detection?!

All Shells (UNIX) are case sensitive by default.

@stefan-kolb stefan-kolb force-pushed the auto-complete branch 2 times, most recently from e8c3413 to f6b85bd Compare April 1, 2016 16:16
@stefan-kolb stefan-kolb changed the title Refactor auto completion [WIP] Refactor auto completion Apr 1, 2016
@stefan-kolb stefan-kolb force-pushed the auto-complete branch 2 times, most recently from a4c5c0f to 2e14f84 Compare April 4, 2016 17:33
@stefan-kolb stefan-kolb changed the title [WIP] Refactor auto completion Refactor auto completion Apr 4, 2016
@stefan-kolb
Copy link
Member Author

Ok, I got something working here now. Massively refactored the code (god was this hard to read 😭).
I'm still not quite happy with the result, but I hope this will be a starting point for more improvements with a readable codebase.

@JabRef/developers WDYT? I'd be happy if you try the version with a few use cases of your own to check whether it is working correctly for you. http://builds.jabref.org/auto-complete/

@stefan-kolb stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 4, 2016
@stefan-kolb stefan-kolb added this to the v3.3 milestone Apr 4, 2016
private List<String> suggestions;
private String currentWord;
private int currentCompletion;
private int realCaretPosition;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make suggestions final and unmodifyable.

@stefan-kolb stefan-kolb added the ui label Apr 4, 2016
@simonharrer
Copy link
Contributor

LGTM 👍

@stefan-kolb
Copy link
Member Author

@JabRef/developers Does it work for you? Should we merge this in or any further suggestions?

@oscargus
Copy link
Contributor

oscargus commented Apr 6, 2016

Would it make sense for the NameAutoCompleter to work for all fields with BibtexSingleFieldProperties.PERSON_NAMES set? I do not really know how it works today, but I seem to recall that it wasn't obvious if the same AutoCompleter was used for both "author" and "editor". (Haven't had time to test it yet.)

@stefan-kolb
Copy link
Member Author

Would be possible, currently it only works on author and editor.

if ("author".equals(fieldName) || "editor".equals(fieldName)) {
             return new NameFieldAutoCompleter(fieldName, preferences);

@stefan-kolb
Copy link
Member Author

I quick checked. Currently only those two fields have the property BibtexSingleFieldProperties.PERSON_NAMES set.

@oscargus
Copy link
Contributor

oscargus commented Apr 6, 2016 via email

@stefan-kolb
Copy link
Member Author

Ah ok. We could do something like this:

if (InternalBibtexFields.getFieldExtras(fieldName).contains(BibtexSingleFieldProperties.PERSON_NAMES)) {
            return new NameFieldAutoCompleter(fieldName, preferences);
        }

But I get NullpointerExceptions in the tests due to the InternalBibtexFields relying on Globals.prefs (InternalBibtexFields.java:125)

@oscargus
Copy link
Contributor

oscargus commented Apr 6, 2016 via email

@koppor
Copy link
Member

koppor commented Apr 7, 2016

What is the purpose of the case-insensitive mode? The detection (only lowercase in word) seems arbitrary to me.

Is this still a valid question? If yes: I was thinking that casing causes more keys to type (shift key). Thus: being lazy should instruct JabRef to search for everything. Being non-lazy (capital letter), I want the tool to search only for these cases as I demand something specific.

In powerline-shell or Windows CMD, the search IMHO also is case-insensitive.

I have not researched for typical algorithms the recent time. When refactoring the auto completion a few years ago, I found nothing useful. However, I did not document my search.

@koppor
Copy link
Member

koppor commented Apr 7, 2016

Quick test:

  • author field
  • Kwantes, PieterM. and Van Gorp, Pieter and Kleijn, Jetty and Rensink, Arend
  • Change V in Van Gorp to v
  • go to the end of the line, type and v.
  • One cannot type v
  • title field
  • Text: Towards Compliance Verification Between Global and Local Process Models
  • Go to the end of the entry
  • type and
  • about is displayed and the accepted characters are abo and not the typed and.

The typed characters have to be kept! The autocompletion must not override the user's input.

I can send you my large bib file privatly. Don't want to share it publicly.

@koppor
Copy link
Member

koppor commented Apr 7, 2016

Exception in thread "AWT-EventQueue-0" java.lang.StringIndexOutOfBoundsException: String index out of range: -67
        at java.lang.String.substring(String.java:1931)
        at net.sf.jabref.gui.autocompleter.AutoCompleteListener.insertKeyAndSuggestion(AutoCompleteListener.java:148)
        at net.sf.jabref.gui.autocompleter.AutoCompleteListener.keyTyped(AutoCompleteListener.java:104)
        at java.awt.Component.processKeyEvent(Component.java:6490)

@stefan-kolb stefan-kolb removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 7, 2016
@stefan-kolb stefan-kolb changed the title Refactor auto completion [WIP] Refactor auto completion Apr 7, 2016
@stefan-kolb stefan-kolb modified the milestones: v3.4, v3.3 Apr 7, 2016
@stefan-kolb stefan-kolb removed this from the v3.4 milestone May 20, 2016
@stefan-kolb stefan-kolb closed this Jul 8, 2016
@stefan-kolb stefan-kolb deleted the auto-complete branch July 8, 2016 09:58
@koppor
Copy link
Member

koppor commented Sep 13, 2016

This PR is superseeded by #1136

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants