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

Add file description to gui and fix sync bug #3210

Merged
merged 9 commits into from
Sep 20, 2017
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We added reordering of file and link entries in the `General`-Tab [3165, comment](https://github.com/JabRef/jabref/issues/3165#issuecomment-326269715)
- We added autcompletion for the `crossref` field on basis of the BibTeX-key. To accept such an autcompleted key as new entry-link, you have to press <kbd>Enter</kbd> two times, otherwise the field data is not stored in the library file.[koppor#257](https://github.com/koppor/jabref/issues/257)
- We added drag and drop support for adding files directly in the `General`-Tab. The dragged files are currently only linked from their existing directory. For more advanced features use the `Add files` dialog. [#koppor#244](https://github.com/koppor/jabref/issues/244)
- We added the file description filed back to the list of files in the `General`-Tab [#2930, comment](https://github.com/JabRef/jabref/issues/2930#issuecomment-328328172)

### Fixed

Expand All @@ -43,7 +44,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We fixed an issue where it was possible to leave the entry editor with an imbalance of braces. [#3167](https://github.com/JabRef/jabref/issues/3167)
- Renaming files now truncates the filename to not exceed the limit of 255 chars [#2622](https://github.com/JabRef/jabref/issues/2622)
- We improved the handling of hyphens in names. [#2775](https://github.com/JabRef/jabref/issues/2775)

- We fixed an issue where an entered file description was not written to the bib-file [#3208](https://github.com/JabRef/jabref/issues/3208)
### Removed
- We removed support for LatexEditor, as it is not under active development. [#3199](https://github.com/JabRef/jabref/issues/3199)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Optional;

Expand Down Expand Up @@ -108,7 +110,11 @@ public void acceptAsLinked() {
}

public Observable[] getObservables() {
return new Observable[] {this.downloadProgress, this.isAutomaticallyFound};
List<Observable> observables = new ArrayList<>(Arrays.asList(linkedFile.getObservables()));
observables.add(downloadOngoing);
observables.add(downloadProgress);
Copy link
Member

Choose a reason for hiding this comment

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

also add isAutomaticallyFound

Copy link

@notuntoward notuntoward Sep 15, 2017

Choose a reason for hiding this comment

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

Just budding in here...

The file list would be cleaner if the filenames weren't shown when there is a description, as was done in JabRef 3.x. If there are very long descriptions, it will still be cluttered, but in my opinion, tooltips aren't an improvement.

The problem with descriptions only in tooltips is that you have to mouse over each one. This makes JabRef harder to use:

  1. It forces you to lift your hands off the keyboard, but more importantly,
  2. You can't scan the descriptions at a glance. Instead, you have to mouse over the first one, then mouse over the next one (which causes the previous description to vanish), and so on.

You can simulate the user experience by having somebody else make you a JabRef entry with, say five files with slightly different descriptions -- this is what your own entries will look like to you, a couple years after you have made them. You won't remember anything.

Now, try to decide which file to open. With tooltips, you see only one description at a time, like you're viewing them through a straw. Now compare that with the JabRef 3.x experience, where all descriptions are visible; with a glance, you can comprehend them all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds reasonable!

Copy link
Member Author

Choose a reason for hiding this comment

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

I now changed the order: Description is shown first and then the file link

Copy link

@notuntoward notuntoward Sep 15, 2017

Choose a reason for hiding this comment

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

Come to think of it, you could even make the filename a tooltip. Once you've got the description, the filename is probably not so useful, but in some situation, you still might want to know it.

But then again, there is the "see everything at a glance" argument I just made...

observables.add(isAutomaticallyFound);
return observables.toArray(new Observable[observables.size()]);
}

public void open() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,9 @@ private void handleOnDragDropped(LinkedFileViewModel originalItem, DragEvent eve

private static Node createFileDisplay(LinkedFileViewModel linkedFile) {
Text icon = MaterialDesignIconFactory.get().createIcon(linkedFile.getTypeIcon());
Text text = new Text(linkedFile.getLink());
Text link = new Text(linkedFile.getLink());
Text desc = new Text(linkedFile.getDescription());

ProgressBar progressIndicator = new ProgressBar();
progressIndicator.progressProperty().bind(linkedFile.downloadProgressProperty());
progressIndicator.visibleProperty().bind(linkedFile.downloadOngoingProperty());
Expand All @@ -161,7 +163,9 @@ private static Node createFileDisplay(LinkedFileViewModel linkedFile) {

HBox container = new HBox(10);
container.setPrefHeight(Double.NEGATIVE_INFINITY);
container.getChildren().addAll(icon, text, progressIndicator, acceptAutoLinkedFile);

container.getChildren().addAll(icon, desc, link, progressIndicator, acceptAutoLinkedFile);

return container;
}

Expand Down
85 changes: 57 additions & 28 deletions src/main/java/org/jabref/model/entry/LinkedFile.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package org.jabref.model.entry;

import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.net.URL;
import java.nio.file.Path;
Expand All @@ -8,6 +11,10 @@
import java.util.Objects;
import java.util.Optional;

import javafx.beans.Observable;
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;

import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.metadata.FileDirectoryPreferences;
import org.jabref.model.util.FileHelper;
Expand All @@ -19,42 +26,47 @@
public class LinkedFile implements Serializable {

private static final LinkedFile NULL_OBJECT = new LinkedFile("", "", "");
private String description;
private String link;
private String fileType;
//We have to mark these properties as transient because they can't be serialized directly
private transient StringProperty description = new SimpleStringProperty();
private transient StringProperty link = new SimpleStringProperty();
private transient StringProperty fileType = new SimpleStringProperty();

public LinkedFile(String description, String link, String fileType) {
this.description = Objects.requireNonNull(description);
this.link = Objects.requireNonNull(link);
this.fileType = Objects.requireNonNull(fileType);
this.description.setValue(Objects.requireNonNull(description));
this.link.setValue(Objects.requireNonNull(link));
this.fileType.setValue(Objects.requireNonNull(fileType));
}

public LinkedFile(String description, URL link, String fileType) {
this(description, Objects.requireNonNull(link).toString(), fileType);
}

public String getFileType() {
return fileType;
return fileType.get();
}

public void setFileType(String fileType) {
this.fileType = fileType;
this.fileType.setValue(fileType);
}

public String getDescription() {
return description;
return description.get();
}

public void setDescription(String description) {
this.description = description;
this.description.setValue(description);
}

public String getLink() {
return link;
return link.get();
}

public void setLink(String link) {
this.link = link;
this.link.setValue(link);
}

public Observable[] getObservables() {
return new Observable[] {this.link, this.description, this.fileType};
}

@Override
Expand All @@ -63,31 +75,48 @@ public boolean equals(Object o) {
return true;
}
if (o instanceof LinkedFile) {

LinkedFile that = (LinkedFile) o;

if (!this.description.equals(that.description)) {
return false;
}
if (!this.link.equals(that.link)) {
return false;
}
return this.fileType.equals(that.fileType);
return Objects.equals(description.get(), that.description.get())
&& Objects.equals(link.get(), that.link.get())
&& Objects.equals(fileType.get(), that.fileType.get());
}
return false;
}

/**
* Writes serialized object to ObjectOutputStream, automatically called
* @param out {@link ObjectOutputStream}
* @throws IOException
*/
private void writeObject(ObjectOutputStream out) throws IOException {
out.writeUTF(getFileType());
out.writeUTF(getLink());
out.writeUTF(getDescription());
out.flush();
}

/**
* Reads serialized object from ObjectInputStreamm, automatically called
* @param in {@link ObjectInputStream}
* @throws IOException
*/
private void readObject(ObjectInputStream in) throws IOException {
fileType = new SimpleStringProperty(in.readUTF());
link = new SimpleStringProperty(in.readUTF());
description = new SimpleStringProperty(in.readUTF());
}

@Override
public int hashCode() {
return Objects.hash(description, link, fileType);
return Objects.hash(description.get(), link.get(), fileType.get());
}

@Override
public String toString() {
return "ParsedFileField{" +
"description='" + description + '\'' +
", link='" + link + '\'' +
", fileType='" + fileType + '\'' +
"description='" + description.get() + '\'' +
", link='" + link.get() + '\'' +
", fileType='" + fileType.get() + '\'' +
'}';
}

Expand All @@ -96,7 +125,7 @@ public boolean isEmpty() {
}

public boolean isOnlineLink() {
return link.startsWith("http://") || link.startsWith("https://") || link.contains("www.");
return link.get().startsWith("http://") || link.get().startsWith("https://") || link.get().contains("www.");
}

public Optional<Path> findIn(BibDatabaseContext databaseContext, FileDirectoryPreferences fileDirectoryPreferences) {
Expand All @@ -105,11 +134,11 @@ public Optional<Path> findIn(BibDatabaseContext databaseContext, FileDirectoryPr
}

public Optional<Path> findIn(List<Path> directories) {
Path file = Paths.get(link);
Path file = Paths.get(link.get());
if (file.isAbsolute() || directories.isEmpty()) {
return Optional.of(file);
} else {
return FileHelper.expandFilenameAsPath(link, directories);
return FileHelper.expandFilenameAsPath(link.get(), directories);
}
}
}
61 changes: 61 additions & 0 deletions src/test/java/org/jabref/model/entry/FileFieldBibEntryTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package org.jabref.model.entry;

import org.jabref.logic.exporter.BibtexDatabaseWriter;
import org.jabref.logic.exporter.SaveException;
import org.jabref.logic.exporter.SavePreferences;
import org.jabref.logic.exporter.StringSaveSession;
import org.jabref.logic.util.OS;
import org.jabref.model.Defaults;
import org.jabref.model.database.BibDatabase;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.metadata.MetaData;

import org.junit.Before;
import org.junit.Test;

import static org.junit.Assert.assertEquals;

public class FileFieldBibEntryTest {

private BibEntry emptyEntry;

@Before
public void setUp() {
emptyEntry = new BibEntry();
emptyEntry.setType("article");
emptyEntry.setChanged(false);
}

@Test
public void testFileFieldSerialization() {
LinkedFile file = new LinkedFile("test", "/home/uers/test.pdf", "PDF");
emptyEntry.addFile(file);

assertEquals("@article{,\n" +
" file = {test:/home/uers/test.pdf:PDF}\n" +
"}", emptyEntry.toString());
}

@Test
public void testFileFieldSerializationDatabase() throws SaveException {
BibDatabase database = new BibDatabase();

LinkedFile file = new LinkedFile("test", "/home/uers/test.pdf", "PDF");
emptyEntry.addFile(file);
database.insertEntries(emptyEntry);

BibtexDatabaseWriter<StringSaveSession> databaseWriter = new BibtexDatabaseWriter<>(StringSaveSession::new);
StringSaveSession saveSession = databaseWriter.savePartOfDatabase(
new BibDatabaseContext(database, new MetaData(), new Defaults()), database.getEntries(),
new SavePreferences());

assertEquals(OS.NEWLINE +
"@Article{,"
+ OS.NEWLINE
+ " file = {test:/home/uers/test.pdf:PDF},"
+ OS.NEWLINE
+ "}" + OS.NEWLINE
+ OS.NEWLINE
+ "@Comment{jabref-meta: databaseType:bibtex;}" + OS.NEWLINE, saveSession.getStringValue());
}
}
16 changes: 12 additions & 4 deletions src/test/java/org/jabref/model/entry/FileFieldWriterTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,17 @@ public void testQuoteNull() {

@Test
public void testEncodeStringArray() {
assertEquals("a:b;c:d", FileFieldWriter.encodeStringArray(new String[][]{{"a", "b"}, {"c", "d"}}));
assertEquals("a:;c:d", FileFieldWriter.encodeStringArray(new String[][]{{"a", ""}, {"c", "d"}}));
assertEquals("a:" + null + ";c:d", FileFieldWriter.encodeStringArray(new String[][]{{"a", null}, {"c", "d"}}));
assertEquals("a:\\:b;c\\;:d", FileFieldWriter.encodeStringArray(new String[][]{{"a", ":b"}, {"c;", "d"}}));
assertEquals("a:b;c:d", FileFieldWriter.encodeStringArray(new String[][] {{"a", "b"}, {"c", "d"}}));
assertEquals("a:;c:d", FileFieldWriter.encodeStringArray(new String[][] {{"a", ""}, {"c", "d"}}));
assertEquals("a:" + null + ";c:d", FileFieldWriter.encodeStringArray(new String[][] {{"a", null}, {"c", "d"}}));
assertEquals("a:\\:b;c\\;:d", FileFieldWriter.encodeStringArray(new String[][] {{"a", ":b"}, {"c;", "d"}}));
}

@Test
public void testFileFieldWriterGetStringRepresentation() {
LinkedFile file = new LinkedFile("test", "X:\\Users\\abc.pdf", "PDF");
assertEquals("test:X\\:\\\\Users\\\\abc.pdf:PDF", FileFieldWriter.getStringRepresentation(file));
}


}