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

Other fields fix and changes #2075

Merged
merged 5 commits into from
Oct 3, 2016
Merged

Other fields fix and changes #2075

merged 5 commits into from
Oct 3, 2016

Conversation

grimes2
Copy link
Contributor

@grimes2 grimes2 commented Sep 27, 2016

These are the claimed improvements:

  • Removed optional fields from other fields (BibTeX)
  • Removed deprecated fields from other fields (BibLaTeX)

See: #2064

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@Siedlerchr
Copy link
Member

I'm sorry, but I am not sure if I get/understand your changes, because what does "OtherFields" refer to?

I have the following tabs. in BibTex:
Required, Optional, General, Abstract, Review, Source
and in Biblatex:
Required, Optional1, Optional2, Deprecated, General, Abstract, Review, Source

Deprecated fields exist for bibtex compatibility, when you import an entry you often get in bibtex.
So these fields are somehow still required.

@grimes2
Copy link
Contributor Author

grimes2 commented Sep 27, 2016

Tab Other fields shows the remaining fields:
otherfields

@grimes2
Copy link
Contributor Author

grimes2 commented Sep 30, 2016

I think I found the bug in #2064. It was not a regression. A new entry editor was not instantiated on changing an entry of the same type. So some "other fields" were not displayed.

@grimes2 grimes2 changed the title Other fields changes Other fields fix and changes Sep 30, 2016
@grimes2
Copy link
Contributor Author

grimes2 commented Sep 30, 2016

Please set label "ready-for-review". Thanks.

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 1, 2016
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

In general the code looks good. I have only no clue for which reason the entry editor originally was cached and if the removal has any performance aspects.

@grimes2
Copy link
Contributor Author

grimes2 commented Oct 2, 2016

The idea was originally to have a better performance. So the entry editor is retained for the same type. This is not a good idea. The entry editor needs to be build completetely new in all cases, because the new entry can have some additional (layout changing) other fields.

@tobiasdiez
Copy link
Member

So the whole entryEditors list can be deleted, right? (because its sole purpose was to cache the entry editor for each type). Moreover, there is a similar code in the showEntry method which differentiates between cached vs not-yet-cached.

@grimes2
Copy link
Contributor Author

grimes2 commented Oct 2, 2016

//To indicate which entry is currently shown.
private final Map<String, EntryEditor> entryEditors = new HashMap<>();

I think, entryEditors stores all the old entry editors and the new entry editor. So I'm not sure, that the sole purpose is to store an entry editor of each type. The similar code in showEntry should also be adjusted, but there is currently no bug report for it.

@grimes2
Copy link
Contributor Author

grimes2 commented Oct 2, 2016

I've checked the code. @tobiasdiez you are right. The whole entry editor caching (for each type) can be removed. I have no performance issues with my databases.

@tobiasdiez tobiasdiez merged commit 76ec8d1 into JabRef:master Oct 3, 2016
@tobiasdiez
Copy link
Member

I noticed a very small latency if you quickly switch from editing one entry to another (for example with the arrow keys). But as I don't think this is a big deal, I'll merge this PR now.

@grimes2
Copy link
Contributor Author

grimes2 commented Oct 3, 2016

Yes, fast scrolling of the entries causes a small latency and moderate CPU usage. But I think, this is no problem. I have an idea for caching the entry editor: entry editor A -> B -> A. A can be cached. But this takes real effect only on heavy usage of the database.

@koppor
Copy link
Member

koppor commented Oct 3, 2016 via email

@tobiasdiez
Copy link
Member

The problem is generating the textfields. So especially in biblatex mode there are around 80 fields which are generated every time. I would not cache the whole editor but just generate the textboxes when the tab is focused.

@grimes2
Copy link
Contributor Author

grimes2 commented Oct 3, 2016

The other field tab is optional (No field -> No tab). I would prefer to create the other fields tab always (No field -> Empty tab). The advantage is, that every entry editor has always the same layout of the tabs. This preserves the active (other fields) tab on changing the entry.

@grimes2 grimes2 deleted the fixotherfields branch October 4, 2016 07:29
@chriba chriba mentioned this pull request Nov 2, 2016
zesaro pushed a commit to zesaro/jabref that referenced this pull request Nov 22, 2016
* Other fields changes

* Fix other fields

* Entry editors cache removed
@koppor koppor mentioned this pull request Sep 3, 2017
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants