Directory pattern bracketed expressions

[sauliusg]

• [ x] Change in CHANGELOG.md described
• [ x] Tests created for changes
• Screenshots added (for bigger UI changes)
• [ x] Manually tested changed features in running JabRef
• Check documentation status (Issue created for outdated help page at help.jabref.org?)
• If you changed the localization: Did you run gradle localizationUpdate?

The offered PR implements functionality discussed in my "File linking functionality update?" proposal (http://discourse.jabref.org/t/file-linking-functionality-update/723). It is now possible to use bracketed expressions (like "[bibtexkey]" or "[year]_[auth]_[firstpage]") consistently in:

• BibTex key generator patterns;
• File search regular expressions (for automatic file linking);
• Import file name patterns;
• Import directory patterns.

To maintain consistent formatting, I have created a new BracketedExpressionExpander class which provides its main methods, 'expandBrackets()'. They essentially contain all bracket expansion functionality moved from BibtexKeyPatternUtil, and the BibtexKeyPatternUtil class now extends BracketedExpressionExpander, and uses its static methods. The BracketedExpressionExpander is also used in FileUtil and RegExpBasedFileFinder, avoiding the previous duplication of functionality in BibtexKeyPatternUtil.makeLabel(...) and RegExpBasedFileFinder.getFieldAndFormat(...) methods (both now use BracketedExpressionExpander.expandBrackets() for dealing with all aspects of bracket expansion in one place).

The bracketed expressions now can be used for Import File name patterns and Import File Directory patterns, providing consistent behavior of all "pattern" tabs in the JabRef "Preferences" dialogue; it is also possible to use underscores in the Import File and Directory patterns (e.g. "[year]_[auth]_[pages]") and to place Import destination directories deeper in the tree (e.g. "by-author/[authIni1]/[auth]" Directory Pattern now works as expected, creating "by-author/P/Pezzotti/2013_Pezzotti_211301.pdf" for the reference: Pezzotti, G. Raman spectroscopy of piezoelectrics Journal of Applied Physics, 2013, 113, 211301).

To retain functionality that was previously implemented by the '\entrytype' Layout pattern, a new bracketed expression "[entrytype]" was created and tested. It is suggested that Layout manager is used exclusively for describing layouts in citation lists, and bracketed expressions are used for describing file system and database entry patterns.

The functionality was tested under LinuxMint 18.1 and Ubuntu 16.04. Unfortunately, I could not test it on Windows hosts since I do not have any of them available for Java testing. I have put in provisions to deal with both '/' and '' directory separators in Directory Patterns, but I would be grateful if someone checks them on a Windows system.

Sincerely,
Saulius

[sauliusg]

Hi,

On 2017-09-30 11:44, Tobias Diez wrote: You favor method b) while I would use a). There are good arguments for both ways. Lets make a deal: I'll present my reasoning for a) and if I can convince you, you change the code accordingly. If not then the code stays like it is right now and I give my +1 for merge 😸

Agreed :)

* In principle, we can do a pre-processing of the pattern in the constructor and then reuse the |expand| method for different entries to save time.

Pre-processing is optimisation, and our interface design decisions should not crucially depend on (hypotetical) optimisations, I would say. Code readability, IMHO, is more important. Also, you can do the proposed optimisation also with the current interface, say by using memoisation. And finally, I can turn your argument on its head an day that I can do reprocessing with the BibEntry to save processing time with different patterns (and then apply my own counter-argument to it (does it get recursive at this stage? :)) So I think No1 is a weak argument, but:

* You can create the |BracketedPattern| from the string in some place (say in the preference class) and then pass it to other methods (for example, the rename cleanup operation). Then only the preference class needs to know how to deserialize the pattern from a string, while the other methods in the chain only care that the are given a |BracketedPattern|. (Background: there was a time, when strings where used for everything in JabRef: File names, entry field names and values, preferences, ... literally everywhere. Since about 2 year we are now trying to encapsulate more and more of these strings in special classes, e. g. |AuthorList| or dedicated preference classes like |LayoutPreferences|. I would like to see the same for the pattern.)

This I buy, I think its a serious consideration.

* If we should decide to change or replace the pattern format, one would only need to change the places where the conversion from string to the pattern happens (which is considerably less overhead then all the places where the pattern is expanded).

Agreed, this is an argument in favour of abstraction of patterns from plain strings.

* If a class is named |BracketedPattern| I would assume that it encapsulates a pattern. 😈

:) Sure, but this is the name you suggested in the first place :) Mine was ...Expander, you remember? Anyway, the names should go after design, not the other way round, shouldn't they? So, to sum up, I think arguments 2) and 3) (1-based) are really in favour of your suggestion. So I implement 'public BracketedPattern(String Pattern)' and '...expand(bibentry)'. Regards, Saulius

…

-- Dr. Saulius Gražulis Vilnius University Institute of Biotechnology, Saulėtekio al. 7 LT-10257 Vilnius, Lietuva (Lithuania) fax: (+370-5)-2234367 / phone (office): (+370-5)-2234353 mobile: (+370-684)-49802, (+370-614)-36366

[sauliusg]

Hi,

On 2017-09-30 11:44, Tobias Diez wrote: a) The pattern as an argument to the constructor and the context information as arguments to the |expandBrackets| method. b) The other way around, that is the context information as arguments to the constructor and the pattern as an argument to the |expandBrackets| method.

I have implemented the (a) variant of the BracketedPattern interface. Please note, however: - I had to implement a default constructor, since BibtexKeyPatternUtil extend BracketedPattern and BibtexKeyPatternUtil so far only has a default constructor; I do not want to refactor even further the use of BibtexKeyPatternUtil -- let's leave it for the next PR if needed :) This means that at the moment there is a possibility to construct a BibtexKeyPatternUtil and BracketedPattern instance without a pattern, and this will later trigger a NullPointerException if used indiscriminately. - For the same reason I have left the expandBrackets(...) static method – it is used by static methods in BibtexKeyPatternUtil and possibly elsewhere. Again, if you want to get rid of those, more refactoring and testing would necessary. At the moment, however, all functionality that was intended for this PR works, is tested, adheres to JabRef coding standards (I hope :). Regards, Saulius

…

-- Dr. Saulius Gražulis Vilnius University Institute of Biotechnology, Saulėtekio al. 7 LT-10257 Vilnius, Lietuva (Lithuania) fax: (+370-5)-2234367 / phone (office): (+370-5)-2234353 mobile: (+370-684)-49802, (+370-614)-36366

[lenhard]

@sauliusg Thanks for the elaboration. Everything you write regarding the bracketed expressions and the ASCII codes seems reasonable and is fine from my point of view. For me, the lack of conditional expressions in the bracketed expressions is also no show stopper and I am not even 100% sure that a follow-up PR is actually needed. We could also wait until someone actually complains about this missing functionality. Nevertheless, you are of course welcome to provide such a PR.

Thanks for offering a make file, but we're happy with gradle, so no need.

I have looked at the new migrations code and tested the functionality locally. As before, it seems to work for the bibtex key patterns. So this PR has its ok for merging from me.

Please note, though, that there might still be hidden problems. Users do very interesting things with bibtex key patterns. Especially when you touch something in the preferences, there might be problems that are almost impossible to foresee. It would be great if you could jump in for a fix if users report problems related to this PR in the future.

[LinusDietz]

okay, this looks good to be tested with real users. since we release 4.0 today, I would kindly ask you to fix the conflict in the changelog, then I would merge this.