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

Enable auto sync per default for Open/Libre Office #6985

Merged
merged 3 commits into from
Oct 12, 2020
Merged

Enable auto sync per default for Open/Libre Office #6985

merged 3 commits into from
Oct 12, 2020

Conversation

IsaacRoles
Copy link
Contributor

Enabling Automatically sync bibliography when inserting citations as the default for Open/Libre Office integrations
Fixes #6957

Pic 1

I have also made changes to the OpenOffice/LibreOffice integration documentation and will submit a pull request for those changes.

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@IsaacRoles
Copy link
Contributor Author

I will wait for feedback before submitting a pull request for the documentation changes.

@IsaacRoles IsaacRoles closed this Oct 5, 2020
@IsaacRoles IsaacRoles reopened this Oct 5, 2020
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! As I explained above, you can just remove the test.

public class OpenOfficeSyncWhenCitingTest {

private static boolean previousValue;
private final JabRefPreferences preferences = JabRefPreferences.getInstance();
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for thinking of a test, but you don't need one here as you are just testing that a variable is stored in the preferences which is not that critical. In JabRef we focus on testing the important things, e.g. model/logic.

To sum up => Just remove the test and it's all good.

And there is a general side effect you have here with the preferences in test , by using the JabRefPreferences Instance Object you are modifying the real preferences values which are also stored/persisted in the registry etc. Therefore, if you need to test something which depends on the preferencesm you need to mock them and "fake" the return values.
See for example:

   importFormatPreferences = mock(ImportFormatPreferences.class, Answers.RETURNS_DEEP_STUBS);
        when(importFormatPreferences.getKeywordSeparator()).thenReturn(',');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback! I went ahead and removed that test. Thanks for your advice, I wasn't sure what would be worth testing.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 8, 2020
Removing unnecessary test
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

For me, it looks good. Not tested though.

@Siedlerchr Siedlerchr merged commit 14bf8f9 into JabRef:master Oct 12, 2020
@Siedlerchr
Copy link
Member

Thanks for your contribution!

Siedlerchr added a commit that referenced this pull request Oct 14, 2020
* upstream/master:
  Jstor Fetcher (#6992)
  Group: "Searching for keywords" searches for a single keyword ==> use singular (#6995)
  Merge parsing of bracketed patterns (#6989)
  6848 fixed the issue of clicking collapse all expanding tree (#6993)
  Enable auto sync per default for Open/Libre Office (#6985)
  Bump unirest-java from 3.11.00 to 3.11.01 (#7001)
  Bump byte-buddy-parent from 1.10.16 to 1.10.17 (#7004)
  Bump lucene-queryparser from 8.6.2 to 8.6.3 (#7002)
  Bump postgresql from 42.2.16 to 42.2.17 (#7005)
  Bump pascalgn/automerge-action from v0.11.0 to v0.12.0 (#7006)
  Bump flowless from 0.6.1 to 0.6.2 (#7003)
@IsaacRoles IsaacRoles deleted the EnableAutoSycPerDefaultOpenOffice branch October 16, 2020 14:36
Siedlerchr added a commit that referenced this pull request Oct 16, 2020
* upstream/master:
  Update journalList.mv
  Update to javafx15 (#7018)
  Squashed 'src/main/resources/csl-styles/' changes from 6fab78b..5297abd
  try to fix DEP issue with official jdk (#7008)
  Jstor Fetcher (#6992)
  Group: "Searching for keywords" searches for a single keyword ==> use singular (#6995)
  Merge parsing of bracketed patterns (#6989)
  6848 fixed the issue of clicking collapse all expanding tree (#6993)
  Enable auto sync per default for Open/Libre Office (#6985)
  Bump unirest-java from 3.11.00 to 3.11.01 (#7001)
  Bump byte-buddy-parent from 1.10.16 to 1.10.17 (#7004)
  Bump lucene-queryparser from 8.6.2 to 8.6.3 (#7002)
  Bump postgresql from 42.2.16 to 42.2.17 (#7005)
  Bump pascalgn/automerge-action from v0.11.0 to v0.12.0 (#7006)
  Bump flowless from 0.6.1 to 0.6.2 (#7003)
Siedlerchr added a commit that referenced this pull request Oct 17, 2020
* upstream/master: (58 commits)
  remove any newlines and spaces in isbn when fetching (#7023)
  add exception to error handler in integrity check
  Update journalList.mv
  Update to javafx15 (#7018)
  Squashed 'src/main/resources/csl-styles/' changes from 6fab78b..5297abd
  try to fix DEP issue with official jdk (#7008)
  Jstor Fetcher (#6992)
  Group: "Searching for keywords" searches for a single keyword ==> use singular (#6995)
  Merge parsing of bracketed patterns (#6989)
  6848 fixed the issue of clicking collapse all expanding tree (#6993)
  Enable auto sync per default for Open/Libre Office (#6985)
  Bump unirest-java from 3.11.00 to 3.11.01 (#7001)
  Bump byte-buddy-parent from 1.10.16 to 1.10.17 (#7004)
  Bump lucene-queryparser from 8.6.2 to 8.6.3 (#7002)
  Bump postgresql from 42.2.16 to 42.2.17 (#7005)
  Bump pascalgn/automerge-action from v0.11.0 to v0.12.0 (#7006)
  Bump flowless from 0.6.1 to 0.6.2 (#7003)
  Rewrite guidelines to Java 15 (#6973)
  Lint CHANGELOG.md
  Removing "BibTeX" when not specific to BibTeX (#6983)
  ...
@teertinker
Copy link
Contributor

Has this feature already been tested for typical use cases? I hope that there are no drawbacks. If the library is synced every time I see two possible problems.

  1. Parsing long manuscripts takes sometimes a lot of time, if the list of references is refreshed each and every time.
  2. The workflow in libreoffice may get disrupted. JabRef pushes many commands to libreoffice while refreshing. As a result, users probably cannot use the edit/undo function anymore.

Since users have the choice, one could still deactivate automatic sync. The problem, however, is that many users do not know that the function exists and what implications it has. I think this is also the reason for issue #6957

I would opt for leaving automatic sync off, because this matches the current design better (we have a refresh button).

@IsaacRoles
Copy link
Contributor Author

I can definitely see your point @teertinker. I don't have enough experience with JabRef to know for sure what the best option is. I can see how this may solve an issue for one particular user (in the case of issue #6957), but cause workflow issues for other users. I needs to be determined what the most common use case is and what is most intuitive to the user.

@koppor
Copy link
Member

koppor commented Oct 22, 2020

@teertinker Maybe, this is a discussion between more "modern" users? I have the impression that many things happen very automatically these days - and the users are more asked to fix somethign if something goes wrong. For instance, IntelliJ just saves the file upon change. Overleaf just compiles the document during writing, ... - I would also wait for feedback. Maybe, there are not that many users crafting long documents in that setting (comapred to users just having 10 to 20 references, such as students). I would also guess/hope/pray that advanced users know about the setting.

@teertinker
Copy link
Contributor

I did a short test using an article with about 20 pages and about 40 references, which should be a typical user case. To test, I inserted a new citation and switched to libreoffice afterwards

performance

  • My new laptop (intel I5) did the sync pretty fast. After switching to libreoffice I can see that there is some text modification going on, but the workflow is not affected.
  • On my old laptop (Intel i3 but 7 years old) I had to wait until sync is completed.

I think the overall performance for the average user should be ok.

workflow

One problem already stated above is that you can't use the undo/redo commands in Libreoffice after inserting a citation because this would only undo/redo the recent internal modifications made by libreoffice. If a user is writing a sentence, cites something and than want's to revert changes by using CTRL+Z this won't work.

I think this is no critical issue but the workflow will be disrupted in some cases without the user knowing exactly why this happens.

automatic sync and sync is not the same

My manuscript contained a reference, that was not included in my current bib file. When I click on the sync button, JabRef will not sync the library but shows an error message that entry xy does not exist. When I switch automatic sync on, however, the library gets synced - except for the missing entry. No error message appears.

While this feature of automatic sync is really good for the workflow and should be kept, it might be an issue for typical user cases. I was assuming, that automatic sync and manual sync have the same functionality and thus would have missed that one reference in my manuscript is corrupted and hence get's not cited properly.

summary

Different users have different needs. I completely understand the advantage of automatic sync when using numbered citations. In my case, I definitely will switch it off - and thanks to the philosophy of JabRef I can. When writing a manuscript I insert citations and do not care about the style of my references. The style (and hence the sync function) is only needed as a last step.

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.

Enable Auto Sync per default for Open/LibeOffice
4 participants