Fix attach file action from contextmenu

[Siedlerchr]

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?

[LinusDietz]

Good catch!

[tobiasdiez]

@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]

@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]

This method is called in many different places

[tobiasdiez]

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]

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]

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]

@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]

@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.

[tobiasdiez]

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

[Siedlerchr]

done