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

Rename "Remote" to "Tele" #9270

Merged
merged 6 commits into from
Nov 5, 2022
Merged

Rename "Remote" to "Tele" #9270

merged 6 commits into from
Nov 5, 2022

Conversation

tobiasdiez
Copy link
Member

The socket server that is run so that other instances of JabRef can detect the running instance were called "Remote". I found this a bit confusing since everything is actually local, so I've renamed it to "Tele".

Besides this rename, I cleaned the code and added a log statement here and there. Only functional change is that the tele server is now only started as part of the other background tasks, i.e. after/during the initial rendering of the main window. Previously, it happened during the CLI, so that even jabref --help started the tele server - and in fact never closed it.

  • Change in CHANGELOG.md described in a way that is understandable for the average user (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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 19, 2022
@@ -7,13 +7,15 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class RemotePreferencesTest {
import org.jabref.logic.tele.TelePreferences;
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.RedundantImportCheck> reported by reviewdog 🐶
Redundant import from the same package - org.jabref.logic.tele.TelePreferences.

@@ -7,13 +7,15 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class RemotePreferencesTest {
import org.jabref.logic.tele.TelePreferences;
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.ImportOrderCheck> reported by reviewdog 🐶
Wrong order for 'org.jabref.logic.tele.TelePreferences' import.

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import org.jabref.logic.tele.TeleUtil;
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.RedundantImportCheck> reported by reviewdog 🐶
Redundant import from the same package - org.jabref.logic.tele.TeleUtil.

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import org.jabref.logic.tele.TeleUtil;
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.ImportOrderCheck> reported by reviewdog 🐶
Wrong order for 'org.jabref.logic.tele.TeleUtil' import.

@@ -7,13 +7,15 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class RemotePreferencesTest {
import org.jabref.logic.tele.TelePreferences;
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.RedundantImportCheck> reported by reviewdog 🐶
Redundant import from the same package - org.jabref.logic.tele.TelePreferences.

@@ -7,13 +7,15 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class RemotePreferencesTest {
import org.jabref.logic.tele.TelePreferences;
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.ImportOrderCheck> reported by reviewdog 🐶
Wrong order for 'org.jabref.logic.tele.TelePreferences' import.

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import org.jabref.logic.tele.TeleUtil;
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.RedundantImportCheck> reported by reviewdog 🐶
Redundant import from the same package - org.jabref.logic.tele.TeleUtil.

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import org.jabref.logic.tele.TeleUtil;
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.ImportOrderCheck> reported by reviewdog 🐶
Wrong order for 'org.jabref.logic.tele.TeleUtil' import.

@koppor
Copy link
Member

koppor commented Oct 24, 2022

DevCall: Please stick with Remote. We don't know the term Tele. We remember Teletext, Telecommunications. In French, the association is with Television. - No one of the developers on the call associated it with "server".

Some know telnet, however, there is an e missing - and it's terminal.

@koppor
Copy link
Member

koppor commented Oct 24, 2022

Seems NOT to fix #9196, does it?

@tobiasdiez
Copy link
Member Author

tobiasdiez commented Oct 24, 2022

Seems NOT to fix #9196, does it?

Was my hope that I find the reason for this issue by going through the code. Not sure if my small change, also fixes #9196. I don't think so but maybe it does. I cannot reproduce #9196 so hard for me to test.

@tobiasdiez
Copy link
Member Author

tobiasdiez commented Oct 24, 2022

DevCall: Please stick with Remote. We don't know the term Tele. We remember Teletext, Telecommunications. In French, the association is with Television.

"Tele" is the greek prefix for "far" or "from far away", e.g. telecommunication = communication over a distance. And in the context of this PR, it would mean "control JabRef from somewhere else", which is pretty much was this code is doing.

No one of the developers on the call associated it with "server".

That was my point. There should be no association to "server". That we use sockets is an implementation detail. We could also write the commands to a file that is watched by another instance, or implement a full http server or write to the registry or whatever. But I think we all agree that "FileWatcher", "ServerWatcher" or "RegistryWatcher" are bad names for this would-be implementation. "Tele" covers all these implementations as well, "Remote" not.

For the same reasons, I find it as a user highly confusing why this feature is under "Network". I would never look there if I want to have only one instance of JabRef. For the sake of limiting the scope of this PR, I didn't touch this though.

I'm very much open to other suggestions, "Tele" was the best I came up with but I'm open to other words as well.

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member

I don't think I understand this, can't we send command line arguments through the client/listener? I was assuming remote ~ remote control. Otherwise, the rest of the network settings wouldn't make sense.

External application access?

@ThiloteE ThiloteE added type: code-quality Issues related to code or architecture decisions shared-database Logging labels Oct 26, 2022
@koppor
Copy link
Member

koppor commented Oct 27, 2022

DevCall: Please stick with Remote. We don't know the term Tele. We remember Teletext, Telecommunications. In French, the association is with Television.

"Tele" is the greek prefix for "far" or "from far away", e.g. telecommunication = communication over a distance. And in the context of this PR, it would mean "control JabRef from somewhere else", which is pretty much was this code is doing.

Based on our IT experience, "tele" is not known and might confuse other developers. We should stay in line with common terms.

For now, I would stick for "remote", because this also offers to open the port on the firewall and let having JabRef controlled remotely. (Doesn't it? I think, we just open the port with no restrictions on the incoming source IP address?)

For the same reasons, I find it as a user highly confusing why this feature is under "Network". I would never look there if I want to have only one instance of JabRef. For the sake of limiting the scope of this PR, I didn't touch this though.

The left menu should not grow more, thus, I currently don't come up with another idea.

@tobiasdiez
Copy link
Member Author

I don't think I understand this, can't we send command line arguments through the client/listener? I was assuming remote ~ remote control. Otherwise, the rest of the network settings wouldn't make sense.

Yes, "remote control" is a good word for it. "Teleoperation" is a synonymy (https://en.wikipedia.org/wiki/Teleoperation and https://en.wikipedia.org/wiki/Telecommand) that I liked since its less overloaded. At least for me "Remote" in an IT context has a server/client connotation, e.g. remote desktops are usually run on a server and the graphical display is shown at the user. This possible source of confusion is also nicely demonstrated by @ThiloteE tagging this PR with "shared database", which is actually a completed different feature but also something I would intuitively think of when I hear "remote" in the context of JabRef.

We should stay in line with common terms.

From https://en.wikipedia.org/wiki/Teleoperation: "[Teleoperation] is similar in meaning to the phrase "remote control" but is usually encountered in research, academia and technology." Seems to be our area, or not?

@ThiloteE
Copy link
Member

Mhm, so if I understand it right, this is code about controlling JabRef from afar? Would it not be possible for multiple users to control the same (remote/tele) JabRef instance at the same time? That is why I thought "shared" database might be a fit, but maybe I am wrong.

@tobiasdiez
Copy link
Member Author

It's about ensuring that only one instance of JabRef is running. Part of the feature is that if you have one Jabref window open, and start another one with some command line arguments (e.g. "open this and that library"), these commands are instead sent to the already running instance. But it's not like that you really have a JabRef server instance that you remote control from another PC or something like that.

@ThiloteE
Copy link
Member

Ah, ok got it. Thanks for the explanation.

@Siedlerchr
Copy link
Member

Renamed Tele back to Remote , otherwise I think the changes look good now

@Siedlerchr Siedlerchr merged commit de57f7f into main Nov 5, 2022
@Siedlerchr Siedlerchr deleted the tele branch November 5, 2022 17:11
Siedlerchr added a commit to yanyanliu1400/jabref that referenced this pull request Nov 7, 2022
* upstream/main: (61 commits)
  Bump jackson-datatype-jsr310 from 2.13.4 to 2.14.0 (JabRef#9357)
  Bump jackson-dataformat-yaml from 2.13.4 to 2.14.0 (JabRef#9356)
  Bump tika-core from 2.5.0 to 2.6.0 (JabRef#9358)
  Refine guideline "Set up a local workspace" (JabRef#9355)
  Adds missing "requires" statement (JabRef#9354)
  Bind "Find unlinked files" default directory to the current library (JabRef#9290)
  Rename "Remote" to "Tele" (JabRef#9270)
  Fixed display of file field adjusting for the dark theme (JabRef#9343)
  Update sorting of entries in maintable by special fields immediately (JabRef#9338)
  New translations JabRef_en.properties (German) (JabRef#9336)
  Fix Abbreviation for Escaped Ampersand (JabRef#9288)
  Fix font size preference not updating in preference dialog 8386 (JabRef#9287)
  New Crowdin updates (JabRef#9333)
  Refresh example styles
  Squashed 'buildres/csl/csl-styles/' changes from 4eee79a..13fd98e
  Bump unirest-java from 3.13.11 to 3.13.12 (JabRef#9330)
  Bump checkstyle from 10.3.4 to 10.4 (JabRef#9331)
  Bump gittools/actions from 0.9.14 to 0.9.15 (JabRef#9332)
  Group context menu presents relevant options depending on number of subgroups (JabRef#9286)
  Removed BibTeX file type and included HTML and Markdown types (JabRef#9318)
  ...
Siedlerchr added a commit that referenced this pull request Nov 8, 2022
* upstream/main:
  Add date format and respective test case (#9310)
  Bump jackson-datatype-jsr310 from 2.13.4 to 2.14.0 (#9357)
  Bump jackson-dataformat-yaml from 2.13.4 to 2.14.0 (#9356)
  Bump tika-core from 2.5.0 to 2.6.0 (#9358)
  Refine guideline "Set up a local workspace" (#9355)
  Adds missing "requires" statement (#9354)
  Bind "Find unlinked files" default directory to the current library (#9290)
  Rename "Remote" to "Tele" (#9270)
  Fixed display of file field adjusting for the dark theme (#9343)
  Update sorting of entries in maintable by special fields immediately (#9338)
  New translations JabRef_en.properties (German) (#9336)
  Fix Abbreviation for Escaped Ampersand (#9288)
  Fix font size preference not updating in preference dialog 8386 (#9287)

# Conflicts:
#	CHANGELOG.md
Siedlerchr added a commit to shafinkamal/jabref that referenced this pull request Nov 11, 2022
* upstream/main: (55 commits)
  Update guidelines-for-setting-up-a-local-workspace.md
  Add link to good first issues (and fix link)
  Refine guidelines (and fix .md file linking)
  Extend ArXiv fetcher results by using data from related DOIs (JabRef#9170)
  New Crowdin updates (JabRef#9366)
  Fix token
  New translations JabRef_en.properties (Chinese Simplified) (JabRef#9364)
  Fix date field change causes an uncaught exception (JabRef#9365)
  Add date format and respective test case (JabRef#9310)
  Bump jackson-datatype-jsr310 from 2.13.4 to 2.14.0 (JabRef#9357)
  Bump jackson-dataformat-yaml from 2.13.4 to 2.14.0 (JabRef#9356)
  Bump tika-core from 2.5.0 to 2.6.0 (JabRef#9358)
  Refine guideline "Set up a local workspace" (JabRef#9355)
  Adds missing "requires" statement (JabRef#9354)
  Bind "Find unlinked files" default directory to the current library (JabRef#9290)
  Rename "Remote" to "Tele" (JabRef#9270)
  Fixed display of file field adjusting for the dark theme (JabRef#9343)
  Update sorting of entries in maintable by special fields immediately (JabRef#9338)
  New translations JabRef_en.properties (German) (JabRef#9336)
  Fix Abbreviation for Escaped Ampersand (JabRef#9288)
  ...

# Conflicts:
#	CHANGELOG.md
#	src/main/java/org/jabref/preferences/AppearancePreferences.java
#	src/main/java/org/jabref/preferences/JabRefPreferences.java
#	src/test/java/org/jabref/gui/theme/ThemeManagerTest.java
Siedlerchr added a commit that referenced this pull request Nov 12, 2022
* upstream/main: (40 commits)
  Update guidelines-for-setting-up-a-local-workspace.md
  Add link to good first issues (and fix link)
  Refine guidelines (and fix .md file linking)
  Extend ArXiv fetcher results by using data from related DOIs (#9170)
  New Crowdin updates (#9366)
  Fix token
  New translations JabRef_en.properties (Chinese Simplified) (#9364)
  Fix date field change causes an uncaught exception (#9365)
  Add date format and respective test case (#9310)
  Bump jackson-datatype-jsr310 from 2.13.4 to 2.14.0 (#9357)
  Bump jackson-dataformat-yaml from 2.13.4 to 2.14.0 (#9356)
  Bump tika-core from 2.5.0 to 2.6.0 (#9358)
  Refine guideline "Set up a local workspace" (#9355)
  Adds missing "requires" statement (#9354)
  Bind "Find unlinked files" default directory to the current library (#9290)
  Rename "Remote" to "Tele" (#9270)
  Fixed display of file field adjusting for the dark theme (#9343)
  Update sorting of entries in maintable by special fields immediately (#9338)
  New translations JabRef_en.properties (German) (#9336)
  Fix Abbreviation for Escaped Ampersand (#9288)
  ...

# Conflicts:
#	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Logging status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants