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

Readd Drop Down content selectors #2301

Merged
merged 39 commits into from
Dec 16, 2016
Merged

Readd Drop Down content selectors #2301

merged 39 commits into from
Dec 16, 2016

Conversation

Siedlerchr
Copy link
Member

First attempt at readding the code for
#2221
Refs #2068

Code does not compile yet, some adjustments need to be made regarding Metadata stuff etc

  • 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?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@matthiasgeiger matthiasgeiger changed the title Readd Drop Down content selectors [WIP] Readd Drop Down content selectors Nov 21, 2016
TODO: Metadata adjustment
@Siedlerchr
Copy link
Member Author

@tobiasdiez There are some things with the Metadata to make it compile again. Could you please have a look? thanks!

@tobiasdiez
Copy link
Member

How to handle the metadata is indeed a problem. I remember that I removed the content selectors mainly because they were the last not-so-easy-to-convert-to-the-new-metadata item. The next few days I will not find the time to assist you here, sorry.

lenhard and others added 8 commits November 28, 2016 13:19
…ntentSelector

* 'master' of https://github.com/Siedlerchr/jabref:
  Rework DBLPFetcher to new structure (#2314)
  Consider dev tag in version comparison (#2312)
  Update diffutils from 1.3.0 to 2.1.1
  Replace usages of Throwable with Exception (#2310)
  Add missing srcDir statement
  improve documentation of save order limitation - see #2305
  Adapt CSL tests to CRLF and LF (#2306)
  Use SPDX license identifiers
  Add key format of #2275 as requirement to CONTRIBUTING.md
  Add fallback exception handler (#2287)
  Update citeproc dependency to 1.0.1 (#2303)
  Remove duplicate code getResolvedFieldOrAlias (#2296)
  Update testCompile dependencies (mockito-core, wiremock)
  Change execution order (#2302)
  Use https for files.jabref.org
  Use compileJava instead of getdeps to decrease build time even more
  Update install4j from 6.1.1 to 6.1.3
  Fix local metadata synchronization
…to contentSelector

* 'contentSelector' of https://github.com/JabRef/jabref:
  Comment out appending content selectors
  Repair content selector dialog
  Add new meta data structure for content selectors
  Fix some compile errors TODO: Metadata adjustment
@Siedlerchr
Copy link
Member Author

Still some language related errors.

@lenhard
Copy link
Member

lenhard commented Nov 28, 2016

The localization keys existed before the feature was removed. I wonder why the revert did not restore them in the language files?

@Siedlerchr
Copy link
Member Author

I did a manual revert, because there were some more changes which would have led to conflicts otherwise.
Was somehow decided.

Fix NPE on JabRef startup
@matthiasgeiger
Copy link
Member

As this was a rather old feature I suppose there were translations for most languages... Can you readd the translated values as well? Or should I go digging in the history? 😉

@lenhard
Copy link
Member

lenhard commented Nov 30, 2016

So I re-implemented parsing and serialization of content selectors and added tests for it. It is probably not the most beautiful fashion, but I need to (want to) link to the old code and not have to re-write the GUI.

Speaking of the GUI: The content selectors seem to be working fine if the MetaData is in the bib file. The GUI in the EntryEditor is not actually beautiful, but I guess it has never been (does anybody feel like polishing the UI?). However, I do not see Options –> Manage content selectors. Maybe that has not been ported from #2068 yet? Maybe someone who is more familiar with the menu than me can have a look? :)

Finally, the test currently say:

net.sf.jabref.logic.help.HelpFileTest > referToValidPage FAILED

Although there seems to be a help page here: http://help.jabref.org/en/ContentSelector Someone certainly has a pointer for that?

@lenhard lenhard added this to the v3.8 milestone Nov 30, 2016
@Siedlerchr
Copy link
Member Author

Siedlerchr commented Nov 30, 2016 via email

@tobiasdiez
Copy link
Member

tobiasdiez commented Nov 30, 2016

I pushed a commit which should take care of the menu item (hopefully, I directly edited it on github).

@lenhard
Copy link
Member

lenhard commented Nov 30, 2016

@tobiasdiez Indeed, it does! Nice :) That means the feature is back!

Regarding the help files, there is CONTENT_SELECTOR("ContentSelector"), in net.sf.jabref.logic.help.HelpFile, so the test really should not fail, should it?

@lenhard
Copy link
Member

lenhard commented Dec 1, 2016

@tobiasdiez I addressed most of your comments. I agree that "content selectors" is probably not the most appropriate name for the stuff this feature does, but it is the name under which it has existed for a long time and is known, at least by some people. I would say that consistency in naming tops appropriateness of the name.

Furthermore, I removed the entry editors update code, as you asked for. The updating of the content selectors did not even work with this code. I tried to replace it with something else and failed. So, for now you have to restart JabRef to get a change in the content selectors to work. Maybe someone from @JabRef/developers can provide a hint on how the refresh of the entry editor should work?

Finally, I will leave the problems in the localization for @Siedlerchr to fix :)

@matthiasgeiger
Copy link
Member

Closing and reopening of the entry editor does not work? (Potentially with selecting another entry?)

@lenhard
Copy link
Member

lenhard commented Dec 1, 2016

@matthiasgeiger Unfortunately not. The changed data makes it into the MetaData, but the GUI does not notice. It is only up-to-date when the MetaData is re-read from disk and the EntryEditor is re-built. Closing and reopening the bib file does the trick, or restarting JabRef

@lenhard
Copy link
Member

lenhard commented Dec 2, 2016

So we would like to get this into v3.8, which hopefully can be released in december. What is missing:

  • correct the language files
  • have selector keywords updated without reloading of the bib file
  • Have the users play around with it to see if it really works

I was hoping that @Siedlerchr could do the first two points, and hopefully soon :-) @Siedlerchr what is your opinion on that?

@Siedlerchr
Copy link
Member Author

Yeah, I will try to work on it. At least I tried to fix the language files, yesterday.
I will continue working on it the weekend

# Conflicts:
#	src/main/java/net/sf/jabref/gui/groups/GroupDialog.java
#	src/test/java/net/sf/jabref/logic/importer/fileformat/BibtexParserTest.java
editor.append(FieldContentSelector.this.delimiter);
}

editor.append(chosen);
// no duplicate -> add it
if (!currentText.contains(chosen)) {
Copy link
Member

@tobiasdiez tobiasdiez Dec 12, 2016

Choose a reason for hiding this comment

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

I think it is better to reuse the KeywordList class here since keyword duplication is not as easy as it appears on first sight. For example, if the field contains "same test, something else" and I would like to add "test", then the simple "contains" solution marks this as duplicate although "same test" is not the same as "test" 😼.

Copy link
Member

Choose a reason for hiding this comment

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

Good point! I changed this to use the keyword list now.

@lenhard lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 13, 2016
# Conflicts:
#	src/main/java/net/sf/jabref/gui/JabRefFrame.java
@Siedlerchr Siedlerchr merged commit 108896f into master Dec 16, 2016
@Siedlerchr Siedlerchr deleted the contentSelector branch December 16, 2016 14:33
@koppor koppor changed the title [WIP] Readd Drop Down content selectors Readd Drop Down content selectors Dec 16, 2016
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.

5 participants