Skip to content

Fix attach file action from contextmenu#3084

Merged
Siedlerchr merged 7 commits into
masterfrom
fixAttachFile
Aug 9, 2017
Merged

Fix attach file action from contextmenu#3084
Siedlerchr merged 7 commits into
masterfrom
fixAttachFile

Conversation

@Siedlerchr

@Siedlerchr Siedlerchr commented Aug 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Return new ArrayList instead of unmodiefable emptyList()

Fixes #3080

  • 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?

@Siedlerchr
Fix attach file action from contextmenu
6059d85 
Return new ArrayList instead of unmodiefable emptyList()
@Siedlerchr
Add unit test
708bd25
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 7, 2017
LinusDietz
LinusDietz approved these changes Aug 7, 2017
View reviewed changes

@LinusDietz LinusDietz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch!

tobiasdiez
tobiasdiez requested changes Aug 7, 2017
View reviewed changes

@tobiasdiez tobiasdiez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@JabRef/developers What are the java conventions about returning immutable vs mutable list?

Personally, I would expect code like entry.getFiles().add(new File()) to add the new file to the internal state of entry if a mutable list is returned, (that is at least how node.getChildren().add(...) works in JavaFX). Thus I'm in favor of always returning an immutable list (i.e. changing line https://github.com/JabRef/jabref/pull/3084/files#diff-bc7e0c121b1a58f1df45d0a6ab2af290R815) and then fixing the callers.

@Siedlerchr

Siedlerchr commented Aug 7, 2017

Copy link
Copy Markdown
Member Author

@tobiasdiez Well, the problem is that the current code always returns an immutable list.
I see no reason why the LInkedFileList should be immutable

    public Optional<FieldChange> addFile(LinkedFile file) {
        List<LinkedFile> linkedFiles = getFiles(); //This was originally breaking because of immutable 
        linkedFiles.add(file);
        return setFiles(linkedFiles);
    }

Siedlerchr
Siedlerchr commented Aug 7, 2017
View reviewed changes
Comment thread src/main/java/org/jabref/model/entry/FileFieldParser.java
return Collections.emptyList();
}

public static List<LinkedFile> parse(String value) {

@Siedlerchr Siedlerchr Aug 7, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This method is called in many different places

@tobiasdiez tobiasdiez Aug 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Returning a mutable list here is ok, since the method is static anyway and thus you don't have the problem that a mutable list exposes the internal state of the object to the outside world.
However, getFiles is not static and only used once...

As I said above, I have no well-founded knowledge about these matters. But probably there a established conventions that we should follow here.

@lenhard lenhard Aug 8, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's fine in this case, because the value of the file field is only stored as a String in BibEntry. On calling getFiles the String is converted into a list, but this list is not stored in BibEntry, so it's not possible to compromise its internal state.

You could call this a "defensive copy", which is fine.

@LinusDietz LinusDietz added this to the v4.0 milestone Aug 8, 2017
lenhard
lenhard approved these changes Aug 8, 2017
View reviewed changes
Comment thread src/main/java/org/jabref/model/entry/FileFieldParser.java
return Collections.emptyList();
}

public static List<LinkedFile> parse(String value) {

@lenhard lenhard Aug 8, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's fine in this case, because the value of the file field is only stored as a String in BibEntry. On calling getFiles the String is converted into a list, but this list is not stored in BibEntry, so it's not possible to compromise its internal state.

You could call this a "defensive copy", which is fine.

@lenhard

lenhard commented Aug 8, 2017

Copy link
Copy Markdown
Member

@tobiasdiez There is no hard Java convention regarding these aspects. You can make the caller fail or you can give him a new list. The only thing that matters is that the internal state of BibEntry is not changed that way.

I don't have a strong opinion as to which solution we should adapt here. Regardless of which one we choose, we should probably document it in code.

@lenhard

lenhard commented Aug 9, 2017

Copy link
Copy Markdown
Member

@Siedlerchr can you expand the JavaDoc comment of BibEntry.getFiles() so that it explains that the returned list is a defensive copy and changes to this list will have no effect on the entry?

With this documentation, the PR can be merged.

@Siedlerchr
Merge remote-tracking branch 'upstream/master' into fixAttachFile
24cbdd1 
* upstream/master:
  Fix #3034: Make font size in entry editor and group panel customizable (#3083)
  Only load the telementry service as a background task if used (#3085)
tobiasdiez
tobiasdiez reviewed Aug 9, 2017
View reviewed changes
Comment thread src/main/java/org/jabref/model/entry/FileFieldParser.java Outdated
sb.append(c);
if ((value.length() > (i + 1)) && (value.charAt(i + 1) == '#')) {
inXmlChar = true;
if ((value != null) && !value.trim().isEmpty()) {

@tobiasdiez tobiasdiez Aug 9, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the previous structure more (early return instead of big if), which was also more inline with typical coding conventions.

@Siedlerchr Siedlerchr Aug 9, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@Siedlerchr Siedlerchr merged commit 56577b5 into master Aug 9, 2017
@Siedlerchr Siedlerchr deleted the fixAttachFile branch August 9, 2017 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tobiasdiez tobiasdiez tobiasdiez requested changes

+2 more reviewers

@LinusDietz LinusDietz LinusDietz approved these changes

@lenhard lenhard lenhard approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Milestone

v4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@Siedlerchr @lenhard @LinusDietz @tobiasdiez