5087 local storage message

[op48]

Feedback welcome! Message can be saved and retrieved from localStorage.

[dan-mi-sun]

This is in reference to this request: #5087

[jhass]

How about Draft? Message can be confusing since we already use that term for the individual messages of a conversation.

[Raven24]

It would be extremely awesome if you could implement the localStorage interaction as a Backbone.sync compatible function. That way it can be re-used in any Backbone model easily. e.g.

app.LocalStorage.sync = function(method, model, options) {  // maybe think of a better name?
// TODO implementation
};

then you can use it in the model like this:

app.models.Draft = Backbone.Model.extend({
  sync: app.LocalStorage.sync
});

[jhass]

I'm also thinking in order to make this more generic we need a good key for the local storage. Maybe something like path+form_id is enough? Wouldn't work for comment forms in the stream I guess.

[op48]

Ok we will look in to the Backbone.sync

[dan-mi-sun]

in initial discussions about this feature, as a first iteration, we said we'd aim for just making this work on the message. as a secord iteration we would look at extending this. which we'd be happy to look into next.

The question is, do we need to do this for this specific PR, given the aim was to hook a message into localStorage...

Thoughts?

[jhass]

If you implement the Backbone.sync stuff it'll probably be extensible enough that you won't have to rewrite it for extensions, so I'd be okay with that for now :)

[dan-mi-sun]

Ok, sounds good. We should have this done today/tomorrow 💃

[jhass]

Great!

[dan-mi-sun]

@jhass @Raven24

We have made some amendments and decoupled the draftable functionality.

We decided not to overwrite Backbone.sync functionality as we can imagine some use-cases where you would not want to overwrite the AJAX default functionality of Backbone.sync

With our proposed feature you can make any model 'draftable' by calling in initialize.

Thoughts?

[jhass]

The idea was not to override Backbone.sync but to implement a new object with a compatible interface. That allows to switch a model between being persisted to the backend or localStorage by overwriting the sync attribute, as @Raven24 demonstrated.

[dan-mi-sun]

@jhass @Raven24

Thanks for the feedback! This is our first attempt at a backbone API :)

The problem we have been trying to solve is: autosaving a message without needing to press save so that if connection is lost or the page closes (or the modal closes), you do not lose your message. (let us know if this is different from your vision!)

Trying to solve this then, some of the drawbacks going down the Backbone.sync route that we could think of were:

Backbone.sync is the function that syncs the front end model with the rails model and will generally be called when either fetching or saving.

Given that we want to autosave, so before pressing anything, it seemed to us to be not inside the object life-cycle of Backbone.sync to save automatically to localStorage.

One way, we could think of using Backbone.sync would be when fetching, sync would first check localStorage and not send an AJAX call to the server, but this is more complex than the implementation we have come up with (for a junior, anyway :) )

We have extracted the code we have written into an API, and will be looking at making aspects of it more generic (such as the key used), so your comments are more than welcome.

Thanks again for your hard work and input.

[jhass]

Once more, the idea is not to replace Backbone.sync but to provide a compatible API that uses LocalStorage as the backend. A model could be switched to that by changing the sync attribute of it from the default of sync: Backbone.sync to sync: app.LocalStorage.sync or whatever the name would be. This means a model would either be persisted to the backend via AJAX or to LocalStorage, not both. So the Draft model would be a candidate that should be persisted to LocalStorage while the Message model would still be persisted to the Backend via AJAX, much like you have it now. The Draft model can then be periodically saved, by calling .save on it, if you wish on each keypress like you do now. The benefits of this approach are that we can switch storage backends for all models simply by defining the sync attribute, for example if a better alternative to LocalStorage comes along, we could switch to it by writing another adapter for it and then changing a single line in the model.

[jhass]

With that design we could also integrate existing solutions such as https://github.com/jeromegn/Backbone.localStorage which is available over bower and thus rails-assets.org.

[dmgarland]

I thought I'd chime in as I'm helping @dan-mi-sun and @op48 and had a hand in the approach we're looking at.

I see the benefit in using the sync method and using more standard naming conventions. I don't have a lot of experience in writing JavaScript plugins and I'm sure there is room for improvement on the API / scoping. I think that our rationale was that in the case of saving a draft, we don't care about fetch, since we presume that would bring in state from the server, and if we wait until we call save, that's too late. We could of course rename our saveDraft method to save, which would remedy the last problem.

Even then, there is still the following conundrum. Suppose we have a backbone model (Message, Comment, Post etc), which we would like to be draftable, up until the point that the user actually saves it. Our thinking was that overriding sync would mean that we would either (a) have to switch between 'draft mode' and 'ajax mode' in the model, or (b) use another model such as Draft, but then transfer the state from a Draft back to the main model.

My understanding of the approach that you have set it presents two considerations. In the former case, I see an unnecessary step in switching between the two models; in the latter case, I see the main issue being that we'd be binding the backbone view events to this draft, which would later need to transfer state to some other model in order to be saved back up to Rails.

I was wondering whether you could elaborate on this and walk us through your approach so that we can see whether we are all on the same page.

[dan-mi-sun]

@Raven24 @jhass did you get a chance to see any of the above? :)

[Raven24]

sorry, it took me a while to respond to this ... here it comes ;)

The idea was to implement a Backbone.sync compatible function (or use one from an existing library) which stores and retrieves model data from/to localStorage. That way it can potentially act as storage backend for any Backbone model we have in the JS app. For now, the only actions it has to support will probably be "read" and "update" (and maybe "delete").

In Backbone models the fetch and save functions delegate to the models sync function, which by default is Backbone.sync. You are right, when you say that there would have to be either a distinct Model for the draft with the localStorage implementation of sync or you'd have to replace the sync function depending on the state.

I consider the first variant to be more straight forward, since you can rely on the models behaving the same way regardless of their state. That means we have to copy data between models, which is fortunately not very hard in Backbone.

// the draft model has only a localsStorage backend
app.models.Draft = Backbone.Model.extend({
  sync: app.LocalStorage.sync
});
// ...
// let's assume there is already a draft in localStorage,
this.draft = new app.models.Draft({id: 'messageDraft');
this.draft.fetch();
// now the LocalStorage.sync function is called and passed the  method 
// parameter 'read' and the model with an id 'messageDraft', the return value 
// should be a hash containing the attributes.
// The model will trigger a "change" event which you can use to update the view.
// ...
// now the user does some more writing or whatever
// the view saves the draft on every keystroke
  events: {
    "keyup input": "saveDraft"  // or similar
  }
// ...
  saveDraft: function() {
    this.draft.set({
      'title': this.$('#inputTitle').val(),
      'text': this.$('#inputText').val()
    });
    this.draft.save();
// here the LocalStorage.sync function is called with method 'update' and
// again with a attributes hash of the model. 
  }

// when the user is finished, and wants to send, 
// transfer attributes to a StatusMessage model
var status_message = new app.models.StatusMessage(this.draft.toJSON());  
  // toJSON makes a shallow copy
status_message.save()  // saves to the server
  .done(function() {  // when that was successful
    this.draft.destroy();  // delete the model data in LocalStorage
  });

I know that is not a very sophisticated or dynamic approach, but in many cases it makes sense not to change the behaviour of an object based on the state. that can make debugging harder and the system gets a little less predictable. So, in this case copying attributes is perfectly fine - every model involved has a single purpose and we have an easy-to-grasp concept of what is happening at any given moment.

[dan-mi-sun]

@Raven24 perfect. Thanks for taking the time to lay that out.

[dan-mi-sun]

@Raven24 & @jhass

I have re-written this feature utelising the backbone.localstorage module.

https://github.com/wegotcoders/diaspora/compare/5087_local_storage_draftable

Code still needs some work, and tests writing but could you clarify whether the new approach is more what you were thinking?

Thanks again for your input so far.

[jhass]

Yes. that looks better, thanks for bearing with us :)

Please pull in the dependency via http://rails-assets.org

gem 'rails-assets-backbone.localStorage', '1.1.13'

//= require backbone.localStorage

[svbergerem]

@dan-mi-sun What is the status here? Still interested in finishing this PR?

[denschub]

Not addressed review issues, merge issues and the lack of authors response leads me to the conclusion it's best to close this PR. Feel free to reopen this at any time and finish your work! Thanks!