Add 'Send to Kindle' feature by aqurilla · Pull Request #9995 · JabRef/jabref

aqurilla · 2023-06-09T06:38:12Z

This fixes #6186 by adding 'Send to Kindle' context & Tools menu actions. The user's Kindle email can be saved in preferences under the 'External Programs' section.

Mandatory checks

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.

Siedlerchr

Codewise looks good so far! Haven't tested it yet.

koppor

In general OK. (And on my machine Send as email stopped working in general, no issue of this PR)

The functionality should only be enabled if there is an attachment:

The whole point is that the file is send ^^. Thus, please add an enablement check of this menu item (in case attachemnt are present, it is enabled. Otherwise, not)

koppor · 2023-06-09T14:45:01Z

+        if (action == StandardActions.SEND_TO_KINDLE) {
+            mailTo.append(preferencesService.getExternalApplicationsPreferences().getKindleEmail());
+            mailTo.append("?Subject=").append(Localization.lang("Send to Kindle"));
+        } else if (action == StandardActions.SEND_AS_EMAIL) {
+            mailTo.append("?Body=").append(rawEntries.toString());
+            mailTo.append("&Subject=");
+            mailTo.append(preferencesService.getExternalApplicationsPreferences().getEmailSubject());
+        }


Just a comment:

This is not object-oriented. Normally, one would crreate an abstract class and two sub classes, where the subclasses differ in that behavior.

However, these are 5 lines of Code only, so OK for me.

koppor · 2023-06-09T14:50:02Z

+        StringBuilder mailTo = new StringBuilder();
+        if (action == StandardActions.SEND_TO_KINDLE) {
+            mailTo.append(preferencesService.getExternalApplicationsPreferences().getKindleEmail());
+            mailTo.append("?Subject=").append(Localization.lang("Send to Kindle"));


I would also add "Send to kindle" as email body. Otherwise, an empty email could be rejected. Who knows...

Sure, will add that. I did try it out using my Kindle email and it does not require a subject or body, but agreed that it would be best to future proof ourselves by adding both anyways 😄

Checking for attachments is enough. Enforced Email configuration could be too hard for newcomers to JabRef.

koppor · 2023-06-09T14:50:36Z

+    private URI getUriMailTo(StringWriter rawEntries, List<String> attachments) throws URISyntaxException {
+        StringBuilder mailTo = new StringBuilder();
+        if (action == StandardActions.SEND_TO_KINDLE) {
+            mailTo.append(preferencesService.getExternalApplicationsPreferences().getKindleEmail());


I assume this also works if the email is not configured?

Currently it does work without a configured email and opens the email dialog with a blank mailTo address. Should this behavior be changed?

Hm, I think it's oaky. one can still add the mail in the email program

Siedlerchr · 2023-06-10T12:43:38Z

Suggestion: Integrate both "Mail" actions into a new submenu Send.
e.g. Send....
+ mail
+ to kindle...

aqurilla · 2023-06-10T20:11:48Z

Added a Send submenu with the email and Kindle options, and added the file attachment check for the Kindle option.
Also converted the different email actions to an abstract class and subclasses as per the recommendation.

koppor · 2023-06-10T21:01:32Z

+        mailTo.append(preferencesService.getExternalApplicationsPreferences().getKindleEmail());
+        mailTo.append("?Body=").append(Localization.lang("Send to Kindle"));
+        mailTo.append("&Subject=").append(Localization.lang("Send to Kindle"));


These are the lines differing at the sub classes.

Proposal: Move up getUriMailTo and create three new abstract methods:

String getEmailAdress()

String getSubject()

String getBody()

In this way, each sub class takes care about its differentiating thing - and the abstract class summarizes the things not being differen.t

Got it thanks, I've updated the PR with these changes

koppor · 2023-06-10T21:03:23Z

+ * Sends the selected entries to any specifiable email
+ * by populating the email body
+ */
+public class SendAsGeneralEmailAction extends SendAsEMailAction {


I don't like General. Maybe Standard is better? --> SendAsStandardEmailAction

koppor · 2023-06-10T21:04:54Z

        <TextField fx:id="eMailReferenceSubject" minWidth="150.0" HBox.hgrow="ALWAYS"/>
    </HBox>
+    <HBox alignment="CENTER_LEFT" spacing="10.0">
+        <Label text="%Send-to-kindle email address"/>


Maybe Email address for sending to Kindle? In this way, the term what to input comes first ("Email address") and then the concretization ("sending to Kindle")

koppor

Thank you for introducing the class hierarchy. One more thing can be moved.

koppor · 2023-06-11T00:35:37Z

+
+    protected abstract String getSubject();
+
+    protected abstract String getBody(StringWriter rawEntries);


I think, the decision is between performance or code readability.

First suggestion - then the stringwriter is converted at the caller

Suggested change

protected abstract String getBody(StringWriter rawEntries);

protected abstract String getBody(String rawEntries);

Second suggestion: As only one sub class needs the bibtex entries, I would pass the List of bibtex entries. The standard email action can do the conversion (you have to move the code from lines 74 to 89) to there. And the kindle action can ignore it.

Sounds good. For the second suggestion we would need both the entries and databaseContext in getBody - is it preferable to get those from stateManager in the subclass instead of passing them in as arguments?

The stateManager can deliver the currently active database and also the currently selected Entries.

Could be a problem though if you right-click on any other entry and you want to send the clicked one. Windows explorer does unselect in this case all the other entries and only the one right-clicked on. So this should be none of your concern in this PR. I think stateManager.getActiveDatabase() and stateManager.getSelectedEntries() should be sufficient.

Thanks for clarifying that! I'll go ahead with this approach

koppor

Thank you for keeping working on this. One nitpick comment ^^. Otherwise: Looks-good-to-me!

koppor · 2023-06-11T19:37:50Z

+        StringWriter rawEntries = new StringWriter();
+        BibWriter bibWriter = new BibWriter(rawEntries, OS.NEWLINE);
+
+        // write the entries via this writer to "rawEntries" (being a StringWriter), which is used later to form the email content


This comment can be removed, as the usage is clear from method namegetBody()

Siedlerchr · 2023-06-12T15:00:19Z

Thanks again for your high quality contributions!

aqurilla · 2023-06-13T00:34:16Z

Thanks, appreciate it!

aqurilla added 3 commits June 8, 2023 23:06

add send-to-kindle action

62c2d0a

add changelog entry

aefd2b6

Merge branch 'main' into fix-for-issue-6186

1d550ef

aqurilla marked this pull request as ready for review June 9, 2023 07:09

Siedlerchr previously approved these changes Jun 9, 2023

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 9, 2023

koppor requested changes Jun 9, 2023

View reviewed changes

aqurilla added 2 commits June 10, 2023 12:58

convert SendAsEMailAction to abstract class

bcfd216

add Send submenu item

0eb9103

aqurilla dismissed Siedlerchr’s stale review via 0eb9103 June 10, 2023 20:00

koppor requested changes Jun 10, 2023

View reviewed changes

move getUriMailTo to abstract class

161443f

koppor requested changes Jun 11, 2023

View reviewed changes

move email body generation code to subclass

e73e865

aqurilla requested a review from koppor June 11, 2023 17:08

koppor requested changes Jun 11, 2023

View reviewed changes

remove comment

a42f536

koppor approved these changes Jun 12, 2023

View reviewed changes

Siedlerchr approved these changes Jun 12, 2023

View reviewed changes

Siedlerchr merged commit 354c55f into JabRef:main Jun 12, 2023


		protected abstract String getSubject();

		protected abstract String getBody(StringWriter rawEntries);

Uh oh!

Conversation

aqurilla commented Jun 9, 2023

Mandatory checks

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Jun 10, 2023

Uh oh!

aqurilla commented Jun 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Jun 12, 2023

Uh oh!

aqurilla commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants