<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 10, 2014 at 9:00 AM, Sebastien Blanc <span dir="ltr"><<a href="mailto:scm.blanc@gmail.com" target="_blank">scm.blanc@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">+1 Dan, thx for all these useful feedback<div>@all : Shall we do separate PR/Tickets for these items or one "bigger" Java Sender overhaul PR ? </div></div></blockquote><div><br></div><div>I am fine with having smaller PRs (easier to review)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="h5"><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 10, 2014 at 5:45 AM, Daniel Bevenius <span dir="ltr"><<a href="mailto:daniel.bevenius@gmail.com" target="_blank">daniel.bevenius@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">+1 I think that making the application id and master secret part of the Sender makes sense. <div><br></div><div>Looking at the code example you posted, I was a little surprised to see that the SenderClient.Builder does not build a SenderClient but instead a JavaSender. Inspecting the code I can see that SenderClient implements JavaSender of course, but having to think about this just truck me as there might be room for improvement. </div><div><br></div><div>Would it be possible to move the methods declared in SenderClient that are missing from JavaSender into that interface? </div><div>For example, it looks like the proxy methods and the truststore methods only exist in SenderClient. </div><div>Was there a reason for leaving them out of the JavaSender interface?</div><div>If we moved the methods up into the interface we could then make the naming consistent, like JavaSender and DefaultJavaSender. I'm lousy at coming up with class/interface names so this might be horrible but would PushSender be a better name for the interface? </div><div><br></div><div>This might be a personal preference but I much prefer using a static method instead when creating a new Builder instance. There is one such static method in SenderClient but the Builder's constructor is still public making the usage above valid. What I'd prefer is that the Builder's constructor be private and provide static methods to start the build process:</div><div><br></div><div><div>PushSender client = DefaultPushSender.withRootServerURL("<a href="http://aerogear.example.com/ag-push" target="_blank">http://aerogear.example.com/ag-push</a>")</div><div> .proxy("proxy", 8080)</div><div> .proxyType(Proxy.Type.HTTP)</div><div> .build();</div></div><div><div>PushSender client = DefaultPushSender.withConfig("/path/on/classpath/push-config.json").build();</div></div><div>PushSender client = DefaultPushSender.withConfig(inputstream).build();<br></div><div><br></div><div>Again, these might be personal preferences and others might disagree but I thought I'd post them just the same incase that offer different view. </div><div><br></div><div>/Dan</div></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On 8 November 2014 13:59, Sebastien Blanc <span dir="ltr"><<a href="mailto:scm.blanc@gmail.com" target="_blank">scm.blanc@gmail.com</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div dir="ltr"><p style="margin-bottom:16px;color:rgb(51,51,51);font-family:'Helvetica Neue',Helvetica,'Segoe UI',Arial,freesans,sans-serif;font-size:16px;line-height:20.479999542236328px;margin-top:0px!important">Hi, </p><p style="margin-bottom:16px;color:rgb(51,51,51);font-family:'Helvetica Neue',Helvetica,'Segoe UI',Arial,freesans,sans-serif;font-size:16px;line-height:20.479999542236328px;margin-top:0px!important">Currently in the Java Sender Library the <code style="font-family:Consolas,'Liberation Mono',Menlo,Courier,monospace;font-size:13.600000381469727px;padding:0.2em 0px;margin:0px;background-color:rgba(0,0,0,0.0392157);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">pushApplicationId</code> and the <code style="font-family:Consolas,'Liberation Mono',Menlo,Courier,monospace;font-size:13.600000381469727px;padding:0.2em 0px;margin:0px;background-color:rgba(0,0,0,0.0392157);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">masterSecret</code> are part of the <code style="font-family:Consolas,'Liberation Mono',Menlo,Courier,monospace;font-size:13.600000381469727px;padding:0.2em 0px;margin:0px;background-color:rgba(0,0,0,0.0392157);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">UnifiedPushMessage</code> object. I would like to move these 2 fields to the <code style="font-family:Consolas,'Liberation Mono',Menlo,Courier,monospace;font-size:13.600000381469727px;padding:0.2em 0px;margin:0px;background-color:rgba(0,0,0,0.0392157);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">Sender</code> object and add it to its Builder API, to have something like :</p><pre style="font-family:Consolas,'Liberation Mono',Menlo,Courier,monospace;font-size:13.600000381469727px;margin-top:0px;margin-bottom:16px;padding:16px;overflow:auto;line-height:1.45;background-color:rgb(247,247,247);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px;word-wrap:normal;color:rgb(51,51,51)"><code style="font-family:Consolas,'Liberation Mono',Menlo,Courier,monospace;padding:0px;margin:0px;background-color:transparent;border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px;word-break:normal;border:0px;display:inline;line-height:inherit;word-wrap:normal">JavaSender defaultJavaSender = new SenderClient.Builder("<a href="http://localhost:8080/ag-push" target="_blank">http://localhost:8080/ag-push</a>")
.pushApplicationId("c7fc6525-5506-4ca9-9cf1-55cc261ddb9c")
.masterSecret("8b2f43a9-23c8-44fe-bee9-d6b0af9e316b")
.build();
</code></pre><p style="margin-top:0px;margin-bottom:16px;color:rgb(51,51,51);font-family:'Helvetica Neue',Helvetica,'Segoe UI',Arial,freesans,sans-serif;font-size:16px;line-height:20.479999542236328px">Why ?</p><ul style="padding:0px 0px 0px 2em;margin-top:0px;margin-bottom:16px;color:rgb(51,51,51);font-family:'Helvetica Neue',Helvetica,'Segoe UI',Arial,freesans,sans-serif;font-size:16px;line-height:20.479999542236328px"><li>Regarding design, these 2 fields are more part of the sender rather than the message itself</li><li>This is a first step to externalize the configuration, so we could have later, for instance, `JavaSender defaultJavaSender = new SenderClient.Config("path.to.config(or resource stream")</li></ul><p style="margin-top:0px;margin-bottom:16px;color:rgb(51,51,51);font-family:'Helvetica Neue',Helvetica,'Segoe UI',Arial,freesans,sans-serif;font-size:16px;line-height:20.479999542236328px">What does it change ?</p><ul style="padding:0px 0px 0px 2em;margin-top:0px;margin-bottom:16px;color:rgb(51,51,51);font-family:'Helvetica Neue',Helvetica,'Segoe UI',Arial,freesans,sans-serif;font-size:16px;line-height:20.479999542236328px"><li>It only concerns the Java Sender, we do not touch the Rest Sender API here.</li><li>A Sender instance is now bounded to a particular PushApp, so if we want to send a message to another PushApp we should :</li><li> Create a new instance of the Sender</li><li> Expose setters to update the config.</li></ul><p style="margin-top:0px;margin-bottom:16px;color:rgb(51,51,51);font-family:'Helvetica Neue',Helvetica,'Segoe UI',Arial,freesans,sans-serif;font-size:16px;line-height:20.479999542236328px">So , wdyt ?</p></div>
<br></div></div>_______________________________________________<br>
aerogear-dev mailing list<br>
<a href="mailto:aerogear-dev@lists.jboss.org" target="_blank">aerogear-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/aerogear-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/aerogear-dev</a><br></blockquote></div><br></div>
<br>_______________________________________________<br>
aerogear-dev mailing list<br>
<a href="mailto:aerogear-dev@lists.jboss.org" target="_blank">aerogear-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/aerogear-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/aerogear-dev</a><br></blockquote></div><br></div></div></div></div></div>
<br>_______________________________________________<br>
aerogear-dev mailing list<br>
<a href="mailto:aerogear-dev@lists.jboss.org">aerogear-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/aerogear-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/aerogear-dev</a><br></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">Matthias Wessendorf <br><br>blog: <a href="http://matthiaswessendorf.wordpress.com/" target="_blank">http://matthiaswessendorf.wordpress.com/</a><br>sessions: <a href="http://www.slideshare.net/mwessendorf" target="_blank">http://www.slideshare.net/mwessendorf</a><br>twitter: <a href="http://twitter.com/mwessendorf" target="_blank">http://twitter.com/mwessendorf</a></div>
</div></div>