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

Resolve crossrefs and strings in main table #1644

Merged
merged 8 commits into from Aug 18, 2016

Conversation

Projects
None yet
4 participants
@oscargus
Contributor

oscargus commented Jul 29, 2016

Based on a discussion in the forum, this is a quick hack to show the possibilities. Can be downloaded and tested from http://builds.jabref.org/resolvedfieldsinmaintable

It would be especially interesting for people with large databases with many strings to test it for performance. A better implementation can be done if this will make it to the released version. At least removing the commented out lines.

  • Change in CHANGELOG.md described
  • Manually tested changed features in running JabRef

@oscargus oscargus added the feature label Jul 29, 2016

@ilippert

This comment has been minimized.

ilippert commented Aug 4, 2016

Am not sure what counts as a large database. My database has currently 9883 entries, out of which 591 use crossref information. I do not perceive any slowdown in daily use.

@oscargus oscargus changed the title from [WIP] Resolve crossrefs and strings in main table to Resolve crossrefs and strings in main table Aug 6, 2016

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 6, 2016

Thanks @ilippert! I'd say that it counts as a large database here, especially considering the amount of crossref entries.

I've updated the code a bit and added a CHANGELOG entry describing the change.

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 8, 2016

OK, I thought it would make sense if one could see which fields are resolved (if nothing else to not confuse some users), so I added that as well. This is a different setting since I think there are different things to want to see required and optional fields and which field that are resolved. At the moment resolved field have precedence if both are enabled.

capture15

Good thing is that the check for optional/required/resolved is now only done if any of the flags are enabled, and, hence, the rendering should be slightly faster compared to before when not enabled.

Non-ASCII_encoded_character_found=
Background_color_for_resolved_fields=

This comment has been minimized.

@koppor

koppor Aug 15, 2016

Member

I would just add the lines of the language strings you added. The other lines should come in in different ways (other PR, via master branch). Otherwise, we will have duplicate lines (refs #1719)

This comment has been minimized.

@oscargus

oscargus Aug 15, 2016

Contributor

Yes, this is probably the reason of not having union merge. However, normally I check these things and unless it takes very long time to merge the PR it doesn't happen that much.

@@ -523,7 +523,7 @@ private String resolveContent(String result, Set<String> usedIds) {
*/
public static Optional<String> getResolvedField(String field, BibEntry entry, BibDatabase database) {
Objects.requireNonNull(entry, "entry cannot be null");
if ("bibtextype".equals(field)) {
if (BibEntry.TYPE_HEADER.equals(field) || "bibtextype".equals(field)) {

This comment has been minimized.

@koppor

koppor Aug 15, 2016

Member

Why do we need two different strings for that? (entryType and bibtextype)?

When searching the code, only BibEntry.TYPE_HEADER should be sufficient. However, it might be the case that my search was too straight-forward.

grabbed_20160815-081924

This comment has been minimized.

@koppor

This comment has been minimized.

@oscargus

oscargus Aug 15, 2016

Contributor

I would guess that in the current master there will be different layout if you have entrytype and bintextype columns. We should probably add a migration that changes any bibtextype column to entrytype. This seems to have been neglected when changing the field name.

This comment has been minimized.

@oscargus

oscargus Aug 15, 2016

Contributor

This has got a bit more clear now. At some stage the name of the entry type was changed from bibtextype to entrytype, but not completely, only in parts of the code, which is evident from the old code and all layout. However, when I tested to use bibtextype in a column nothing was shown, not clear why though. Anyway, the changes in #1651 are unified in this PR and maybe we should simply remove "bibtextype", but better wait for a while. No need for any migration as "bibtextype" didn't work in a column anyway and we do not want to modify peoples layout files...

@@ -144,4 +132,26 @@ public JLabel getHeaderLabel() {
return new JLabel(getDisplayName());
}
}
public boolean isResolved(BibEntry entry) {

This comment has been minimized.

@koppor

koppor Aug 15, 2016

Member

Please add a JavaDoc comment indicating the intention of this method. Currently, it seems if the first field of entry contains something, it returns true.

This comment has been minimized.

@oscargus

oscargus via email Aug 15, 2016

Contributor

This comment has been minimized.

@koppor

koppor Aug 15, 2016

Member

I'm trying to understand why one needs a flag if the entry is resolved at all. Don't I need to in a field-bases?

This comment has been minimized.

@oscargus

oscargus via email Aug 15, 2016

Contributor

This comment has been minimized.

@koppor

koppor Aug 15, 2016

Member

I think, that one needs the information on a per-field, not on a per-entry basis. As far as I understood, I get it for both cases: resolved fields are highlighted and there is an indication on the complete entry. - This is OK 😇

This comment has been minimized.

@oscargus

oscargus Aug 15, 2016

Contributor

Well, there is nothing saying if it is a crossref, string, or alias
resolving at the moment. Hence, the month field will mark gray with content
"Jan" independent of:

  • the entry not having a month/date, but the crossref does
  • it says #jan# in the field
  • the entry has a date xxxx-01-xx

Indeed it may (initially) be a bit confusing for the user when the entry
editor says something different, that is why I wanted a marker. If tooltips
would work in the table one could (in the long run) add a tooltip saying
where the information origins. Also, the idea of having a "resolved"
grayed/italic default text in the entry editor for empty fields may be
worthwhile.

return false;
} else {
entryContent = entry.getFieldOptional(field);
content = BibDatabase.getResolvedField(field, entry, database.orElse(null));

This comment has been minimized.

@koppor

koppor Aug 15, 2016

Member

When checking the code, it seems that this method either returns the content of the field or, if it can be resolved, the content of the resolved field. Correct me, if I'm wrong.

This comment has been minimized.

@oscargus

oscargus via email Aug 15, 2016

Contributor

This comment has been minimized.

@koppor

koppor Aug 15, 2016

Member

Oh. OK. Maybe just rename entryContent -> plainFieldContent and content to resolvedFieldContent.

This comment has been minimized.

@oscargus

oscargus Aug 15, 2016

Contributor

I didn't really understand what I commented on. You suggested naming still makes sense, but the method returns false if the content of the field is the same as the resolved content of the field, i.e., no string, crossrefs, or alias fields are used.

@koppor

This comment has been minimized.

Member

koppor commented Aug 15, 2016

Could you add an example bib file (maybe artificially generated) to show the effects of your change. I see your screenshot, but I want to try it for myself. -- Up to now, I did not use resolved fields at all as it seems that bibtex/biblatex resolves them differently and I don't know, how far JabRef follows this. Moreover, copy and paste does not work intuitively, but this is addressed at #1677.

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 15, 2016

Here is a bib-file to show a bit (must use extension .txt for GitHub). Make sure you show year, date, and month (this also shows that we should try to normalize the month display based on database type, I think).
resolvedatabase.txt

@koppor

This comment has been minimized.

Member

koppor commented Aug 17, 2016

I checked it with normal JabRef config. The "background color for resolved fields" doesn't seem to have any effect, but it should.

normal operation

sp160817_081811

resolved

resolved

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 17, 2016

You mean it should be enabled by default or that enabling it doesn't lead to any changes? There should be a light-gray background...

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 17, 2016

OK, I think I understand what you meant. Either way, I fixed a problem related to that now.

Which leads to another question: how should we deal with marking mandatory fields if there are, say, "author/editor" and exactly one of them is mandatory, both are mandatory, or it is mandatory to have one of them? Now, the column name is simply compared with the list of mandatory fields, so it only works for single field columns.

@koppor

This comment has been minimized.

Member

koppor commented Aug 17, 2016

The "Background color for resolved fields" seems to have no effect here:

grabbed_20160817-213158

Regarding "author/editor" -> Why not splitting the name using "/" and then checking each of them during the column name comparison? I know, we should use some cool data structures. Since we are aiming for JavaFX, this can be that dirty IMHO.

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 17, 2016

You need to tick "Color codes for resolved fields".

The splitting can be easily done. The question is just what the logic should be? Say that a column shows "author/doi", "author" is a mandatory field, but only the "doi" is present. Color?

@@ -28,6 +28,8 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- Added ISBN integrity checker
- Added filter to not show selected integrity checks
- The contents of `crossref` and `related` will be automatically updated if a linked entry changes key
- The information shown in the main table now resolves crossrefs and strings

This comment has been minimized.

@koppor

koppor Aug 17, 2016

Member

Remove this line - the next line contains the same information (and more)

This comment has been minimized.

@oscargus

oscargus Aug 17, 2016

Contributor

Not to mention the grammatical error in the next line... I would like to blame union merge. ;-)

@@ -1680,3 +1680,5 @@ You_must_enter_at_least_one_field_name=
Non-ASCII_encoded_character_found=
Toggle_web_search_interface=
Background_color_for_resolved_fields=
Color_codes_for_resolved_fields=

This comment has been minimized.

@koppor

koppor Aug 17, 2016

Member

Shouldn't it be singular? "Color code for resolved fields"?

This comment has been minimized.

@oscargus

oscargus Aug 17, 2016

Contributor

You are probably right. I thought about it when I wrote it, think of consistency and "fields", but you are correct, there is only one color. I'll fix it tomorrow.

@koppor

This comment has been minimized.

Member

koppor commented Aug 17, 2016

Is it possible to enable "Color codes for resolved fields" as soon as one changes the color for resolved fields?

Color: If (at least) one mandatory field is missing: Color it red. Otherwise, normal color. In your example, the field would be red: Some content is there, but not the mandatory one.

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 18, 2016

Currently, only the number is colored red, so I do not think that is actually the answer. There may, for example, be another column with "editor" which is present and the entry is a book. I know it sounds complicated (and made up examples), but I thought about it and I do not really see what logic should be used for marking required and optional fields when there are multiple fields in one column.

Maybe the easiest way is to just enable the resolved entry marking by default? While it is possible, with the current code structure it will be quite hacky (since the buttons are initialized in one loop, I will have to single it out, etc, now, since it is the last one, at the moment, it is quite reasonable, but not really a nice solution).

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 18, 2016

For consistency one should in that case also do the same thing for the other color codes (required/optional) and grid lines. Maybe one can pass a JCheckButton to the ColorButton as an extra argument, passing null for all other cases?

@oscargus

This comment has been minimized.

Contributor

oscargus commented Aug 18, 2016

I added automatic enabling for those three cases...

@koppor koppor merged commit f5c8419 into master Aug 18, 2016

2 of 3 checks passed

codecov/project 28.76% (-0.02%) compared to 7212bcd
Details
codacy/pr Good work! A perfect pull request.
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@koppor koppor deleted the resolvedfieldsinmaintable branch Aug 18, 2016

Siedlerchr added a commit to Siedlerchr/jabref that referenced this pull request Aug 18, 2016

Merge branch 'master' into office07
* master:
  Fix imports
  Rename NewFileDialog -> FileDialog
  Also cancel duplicate finder workflow on close button
  Removed/moved preferences which are constants
  implements JabRef#1767: Add Help Button to access new help page
  Fixed BibTeXMLImporter
  Set more default file filters in dialogs JabRef#1763
  Resolve crossrefs and strings in main table (JabRef#1644)
  Rewrite bibtexml importer with JAXB parser (JabRef#1666)
  Moved a few more initialization to GUIGlobals.init() (JabRef#1756)
  Added program to generate a table of all characters and fixed some characters (JabRef#1766)
  Improve focus of the maintable after a sidepane gets closed (JabRef#1741)
  Table row height adjusts on Windows as the font scales with the menu (JabRef#1623)
  Added more characters to converters (JabRef#1761)

ayanai1 added a commit to ayanai1/jabref that referenced this pull request Sep 5, 2016

Resolve crossrefs and strings in main table (JabRef#1644)
* Resolve crossrefs and strings in main table

* Improved code and added CHANGELOG entry

* Added special background color for resolved fields

* Fixed comments

* Fixed mandatory field coloring

* Fixed CHANGELOG entry

* Changed wording and made resolved coloring default

* Added automatic enabling of color codes and grid lines on editing color
@Dellu

This comment has been minimized.

Dellu commented Dec 8, 2016

Here is one suggestion about the Crossref:
It would be nice if the BookTitle could be displayed, rather than the Title

That, is when the parent entry has only BookTitle (not the title--due to the problems of Bibtex; we cannot use the Title unless we migrate to BibLatex).

Here is a Proceeding with the Booktitle: Collection

Look at the following link: the Booktitle is not visible:
crossrefered

https://snag.gy/JEk4dC.jpg

@ilippert

This comment has been minimized.

ilippert commented Dec 8, 2016

@Dellu
it might be the case that you can achieve the desired effect by setting in the jabref preferences|Entry table columns| a field name to e.g. title/booktitle/chapter.
The effect is that if available the first, and if not available the second field name (and so on) content for the field is shown.

@ilippert

This comment has been minimized.

ilippert commented Dec 8, 2016

sorry, yes, now I see more clearly. My fault for commenting too swiftly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment