-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add 'Send to Kindle' feature #9995
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codewise looks good so far! Haven't tested it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
StringBuilder mailTo = new StringBuilder(); | ||
if (action == StandardActions.SEND_TO_KINDLE) { | ||
mailTo.append(preferencesService.getExternalApplicationsPreferences().getKindleEmail()); | ||
mailTo.append("?Subject=").append(Localization.lang("Send to Kindle")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add "Send to kindle" as email body. Otherwise, an empty email could be rejected. Who knows...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for attachments is enough. Enforced Email configuration could be too hard for newcomers to JabRef.
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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this also works if the email is not configured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently it does work without a configured email and opens the email dialog with a blank mailTo address. Should this behavior be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think it's oaky. one can still add the mail in the email program
Suggestion: Integrate both "Mail" actions into a new submenu Send. |
mailTo.append(preferencesService.getExternalApplicationsPreferences().getKindleEmail()); | ||
mailTo.append("?Body=").append(Localization.lang("Send to Kindle")); | ||
mailTo.append("&Subject=").append(Localization.lang("Send to Kindle")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it thanks, I've updated the PR with these changes
* Sends the selected entries to any specifiable email | ||
* by populating the email body | ||
*/ | ||
public class SendAsGeneralEmailAction extends SendAsEMailAction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like General
. Maybe Standard
is better? --> SendAsStandardEmailAction
@@ -21,6 +21,10 @@ | |||
<Label text="%Subject for sending an email with references"/> | |||
<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"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for introducing the class hierarchy. One more thing can be moved.
|
||
protected abstract String getSubject(); | ||
|
||
protected abstract String getBody(StringWriter rawEntries); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, the decision is between performance or code readability.
First suggestion - then the stringwriter is converted at the caller
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying that! I'll go ahead with this approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for keeping working on this. One nitpick comment ^^. Otherwise: Looks-good-to-me!
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment can be removed, as the usage is clear from method namegetBody()
Thanks again for your high quality contributions! |
Thanks, appreciate it! |
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
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)