-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Changes from 7 commits
55e3d1d
d985361
23c6ac6
52b5b42
3197713
e05f8b0
f767625
c517843
4cf1a6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,7 @@ public LinkedFilesEditorViewModel(String fieldName, AutoCompleteSuggestionProvid | |
text, | ||
LinkedFilesEditorViewModel::getStringRepresentation, | ||
this::parseToFileViewModel); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very minor, but as far as I can see this blank line is unnecessary. |
||
} | ||
|
||
private static String getStringRepresentation(List<LinkedFileViewModel> files) { | ||
|
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; | ||
|
@@ -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; | ||
|
@@ -19,42 +26,48 @@ | |
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 | ||
|
@@ -63,31 +76,50 @@ 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(); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very minor, but as far as I can see this blank line is unnecessary. |
||
} | ||
|
||
/** | ||
* 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()); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very minor, but as far as I can see this blank line is unnecessary. |
||
} | ||
|
||
@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() + '\'' + | ||
'}'; | ||
} | ||
|
||
|
@@ -96,7 +128,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) { | ||
|
@@ -105,11 +137,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); | ||
} | ||
} | ||
} |
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()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also add
isAutomaticallyFound
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...