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

WIP: Autosave posts #1063

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
8 participants
@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
@@ -0,0 +1,24 @@
function phpbb_autosave () {
try {
window.localStorage['phpbb_post'] = $('#message')[0].value;

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 10, 2012

Contributor

Just use the .value() function

This comment has been minimized.

Copy link
@Fyorl

Fyorl Nov 11, 2012

Author Contributor

What .value() function?

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
// If we have data in phpbb_post when the page loads we can assume it's ok
// to load their autosave
if (window.localStorage['phpbb_post']) {
$('#message')[0].value = window.localStorage['phpbb_post'];

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 10, 2012

Contributor

Again, use the .value() function here

@callumacrae

View changes

phpBB/styles/prosilver/template/posting_layout.html Outdated
@@ -87,4 +87,6 @@
</script>
<!-- ENDIF -->

<script src="{T_ASSETS_PATH}/javascript/autosave.js"></script>

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 10, 2012

Contributor

Use INCLUDEJS

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
$('#message')[0].value = window.localStorage['phpbb_post'];
}

// Autosave every 30 seconds

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 10, 2012

Contributor

I don't think this comment is needed. The code itself is readable, and it just seems like it is the kind of comment where someone will change the amount of time, but forget to change the comment.

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
}

// If we have data in phpbb_post when the page loads we can assume it's ok
// to load their autosave

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 10, 2012

Contributor

I think you should be saving the date, too. If they come back a few months later, they won't want the post back and it could be confusing.

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
return;
}

var loc = $(location)[0];

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 10, 2012

Contributor

Why are you doing this?

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated

var loc = $(location)[0];
if (loc.search.match(/f=(\d+)/)) {
var f = RegExp.$1;

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 10, 2012

Contributor

Using RegExp.$1 is fairly bad practice, iirc. Save the .match call to a variable and then refer to that instead.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 10, 2012

Contributor

You should also be declaring your variables at the top of the function

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated

for (var key in data) {
dt = new Date((key + phpbb_autosave_tz_offset) * 1000);
str += "\n" + (keys.length + 1) + ') ' + phpbb_autosave_timestr(dt);

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

Single quotes (there is no difference in JS)

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
if (keys.length < 2) {
return data[keys[0]];
} else {
var choice;

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

This should be at the very top of the function. It doesn't affect behaviour or performance at all as they're all hoisted anyway.

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
var choice;

while (true) {
choice = prompt(str);

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

Please don't use prompt!

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
choice = prompt(str);

try {
choice = parseInt(choice);

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

Number(choice) would be more appropriate here, I think.

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated

function phpbb_autosave_save (key) {
try {
window.localStorage[key] = $('textarea[name=message]').attr('value');

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

$('textarea[name=message]').value()

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
function phpbb_autosave_prompt (data) {
var str = phpbb_autosave_prompt_msg;
var dt;
var keys = [];

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

There should only be one var statement:

var str = phpbb_autosave_prompt_msg,
    keys = [],
    dt;
@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
}
}

function phpbb_autosave_save (key) {

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

This should be attached to the phpbb object instead of being a function

@@ -0,0 +1,5 @@
<script>
var phpbb_autosave_prompt_msg = '{LA_AUTOSAVE_PROMPT}';
var phpbb_autosave_tz_offset = {TIMEZONE_OFFSET};

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

These should also be attached to the phpbb object - it prevents pollution of the global namespace.

This comment has been minimized.

Copy link
@Fyorl

Fyorl Nov 11, 2012

Author Contributor

The phpbb object only becomes available in core.js which is included in the overall_footer and after this code.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

Ah, fair enough.

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated

function phpbb_autosave_timestr (dt) {
var hrs = dt.getUTCHours();
var mins = dt.getUTCMinutes();

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

Commas would be preferred, again

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated

// Just store a variable without the creation time for convenience
var key_no_creation = key;
key += '_' + $('input[name=creation_time]').attr('value');

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

.value again as above

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
key += window.location.search.replace(/.*?\W(f|t|p)=(\d)+.*?/g, '_$2');

// Just store a variable without the creation time for convenience
var key_no_creation = key;

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

Vars at top of function

@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
}
}

$('textarea[name=message]').attr('value', phpbb_autosave_prompt(data));

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor
$('textarea[name=message]').value(phpbb_autosave_prompt(data));
@callumacrae

View changes

phpBB/assets/javascript/autosave.js Outdated
for (var storage_key in window.localStorage) {
var explode = storage_key.split('_');
if (key_no_creation === explode.slice(0, -1).join('_')) {
data[parseInt(explode.pop())] = window.localStorage[storage_key];

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 11, 2012

Contributor

You shouldn't need to parseInt this, as it casts it to string for the key anyway.

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 13, 2012

Contributor

Did you test this line? Did it really work? Is this information really persistent? Is it according to the w3c's spec?

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 13, 2012

Contributor

Why wouldn't it work?

On 13 Nov 2012, at 09:30, brunoais notifications@github.com wrote:

In phpBB/assets/javascript/autosave.js:

  • var key = window.location.host + window.location.pathname;
  • // Extract the f=, t= or p= from the URL and separate with underscores
  • key += window.location.search.replace(/.?\W(f|t|p)=(\d)+.?/g, '_$2');
  • // Just store a variable without the creation time for convenience
  • var key_no_creation = key;
  • key += '_' + $('input[name=creation_time]').val();
  • // If we have data in localStorage when the page loads we can assume it's
  • // ok to load their autosave
  • var data = {};
  • for (var storage_key in window.localStorage) {
  •   var explode = storage_key.split('_');
    
  •   if (key_no_creation === explode.slice(0, -1).join('_')) {
    
  •       data[parseInt(explode.pop())] = window.localStorage[storage_key];
    
    Did you test this line? Did it really work? Is it really persistent and is it according to the w3c's spec?


Reply to this email directly or view it on GitHub.

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 13, 2012

Contributor

Because it's a parameter in the object and not a method like I find in the spec.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 13, 2012

Contributor

It's the old syntax.

On 13 Nov 2012, at 09:47, brunoais notifications@github.com wrote:

In phpBB/assets/javascript/autosave.js:

  • var key = window.location.host + window.location.pathname;
  • // Extract the f=, t= or p= from the URL and separate with underscores
  • key += window.location.search.replace(/.?\W(f|t|p)=(\d)+.?/g, '_$2');
  • // Just store a variable without the creation time for convenience
  • var key_no_creation = key;
  • key += '_' + $('input[name=creation_time]').val();
  • // If we have data in localStorage when the page loads we can assume it's
  • // ok to load their autosave
  • var data = {};
  • for (var storage_key in window.localStorage) {
  •   var explode = storage_key.split('_');
    
  •   if (key_no_creation === explode.slice(0, -1).join('_')) {
    
  •       data[parseInt(explode.pop())] = window.localStorage[storage_key];
    
    Because it's a parameter in the object and not a method like I find in the spec.


Reply to this email directly or view it on GitHub.

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 13, 2012

Contributor

Because you are writting to a parameter in an object. It could be
something that is lost when the object is destroyed just what happens
when you override a DOM object (I'm not referring to the window object
or the document object. I'm referring to a Node that can be removed from
the document itself). I have absolutely no information about the old
method you are referring to. Anyway (unless there's a really good reason
for not to upgrade) if it's an old method, if should be changed so that
is follows the new method.

@@ -0,0 +1,93 @@
phpbb.autosave_prompt = function (data) {

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 13, 2012

Contributor

I didn't understand what is this prompt about...

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 13, 2012

Contributor

Did you look at the language entry? Did you test it?

This comment has been minimized.

Copy link
@erikfrerejean

erikfrerejean Nov 13, 2012

It should be clear what this does without having to dig through language entries or so.


phpbb.autosave_save = function (key) {
try {
window.localStorage[key] = $('textarea[name=message]').val();

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 13, 2012

Contributor

Don't use this. It may not store properly (or store at all). Use the correct methods for it. (https://developer.mozilla.org/en-US/docs/DOM/Storage#localStorage)


Don't search for the textarea all the time. Store the wrapped node in a variable and use that variable instead.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 13, 2012

Contributor

Course it will store properly, it's the old syntax.

I'm not sure that I can see the point in caching the object. It won't affect anything in any way unless the function is being ran more than probably 20 times a second

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 13, 2012

Contributor

Old syntax is meant for old and not updated programs. New programs are supposed to be made as updated as it is possible.


DOM operations are slow. That's a fact. By working this way you are not only using the DOM repeatedly (even though it's not multiple times per second) and also the trash collector which is also not something that is that cheap. The real problem does not lye in this happening 1ce. Lies in this happening many times, in many places, in the code. All those together it becomes significant (specially in slower systems). Also remember that jQuery itself is really very slow and uses a lot of objects to do that search for the node and to encapsulate it.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 13, 2012

Contributor

It is running once every thirty seconds. It's a single DOM operation. It won't make any difference.

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 13, 2012

Contributor

Read the jQuery source code, then.
You'll see that it is (these are the minimum numbers, dunno really if
there's more and with IE8 there's really a whole lot more) 8 DOM
operations, 2 regex matches, 1 string parsing and 10 objects. To know
the exact numbers I'd need to do more checks on how jQuery works for
that specific operation, though.

Oh! And in IE8, jQuery navigates through all nodes in the DOM with that
operation. So the min DOM operations is about 3 + the number of nodes in
the document node. And I still don't really know what are the conditions
that let jQuery use the querySelectorAll() method. There are conditions
that it calculates such that makes it does not using that method. When
that happens, 8 DOM operations is just a dream. The real number is much
higher.

So, IMO doing that operation once per page load is completely acceptable
due to how much it simplifies our code but doing it more than that is abuse.

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 14, 2012

Contributor

Whoops, it is alternative, not old: https://developer.mozilla.org/en-US/docs/DOM/Storage#localStorage

Storage is defined by the WhatWG Storage Interface as this:

interface Storage {
readonly attribute unsigned long length;
[IndexGetter] DOMString key(in unsigned long index);
[NameGetter] DOMString getItem(in DOMString key);
[NameSetter] void setItem(in DOMString key, in DOMString data);
[NameDeleter] void removeItem(in DOMString key);
void clear();
};

This comment has been minimized.

Copy link
@brunoais

brunoais Nov 14, 2012

Contributor

You forgot this part:

Note: Although the values can be set and read using the standard JavaScript property access method, using the getItem and setItem methods is recommended.

Why don't we use the recommended way that follows the interface, then?
Even though using the setter as an object attribute may (and probably does) work, I still believe that using the public interface, the recomended way by MDN, is the best way to go.
I read the W3c's page about the Storage interface with some attention and used search to try to find and I didn't find anything about storing values using the attributes of the Storage object itself. It can be a feature that is not stable or available in as much browsers as the ones that implement the Storage interface defined by W3c.

This comment has been minimized.

Copy link
@senky

senky Dec 27, 2012

Contributor

Maybe brunoais is right in storing node in variable. Well, it maybe does not make big difference now, but it is at least more extensible for the future.

btw, shouldn't quotes be used in selector?

$('textarea[name="message"]').val();

(I know it will work without them, but according to my knowledge, quotes should be used if it is string)

This comment has been minimized.

Copy link
@brunoais

brunoais Dec 28, 2012

Contributor

(I know it will work without them, but according to my knowledge, quotes should be used if it is string)

By the CSS selector rules, one must use the quotes in order to the selector to work. Anyway, this is a selector specially made for jQuery, so it may follow other rules. I made a quick search in the jQuery's manual, I didn't find anything about that specific part. Probably I'd have to search deeper.

window.localStorage[key] = $('textarea[name=message]').val();
} catch (e) {
// Quota exceeded, should inform the user that their autosave isn't
// working

This comment has been minimized.

Copy link
@callumacrae

callumacrae Nov 13, 2012

Contributor

"WIP"

On 13 Nov 2012, at 09:29, brunoais notifications@github.com wrote:

In phpBB/assets/javascript/autosave.js:

  •           continue;
    
  •       }
    
  •       break;
    
  •   }
    
  •   return data[keys[choice - 1]];
    
  • }
    +};

+phpbb.autosave_save = function (key) {

  • try {
  •   window.localStorage[key] = $('textarea[name=message]').val();
    
  • } catch (e) {
  •   // Quota exceeded, should inform the user that their autosave isn't
    
  •   // working
    
    Better solve this somehow. How about making a div that has that written appear under the textarea?


Reply to this email directly or view it on GitHub.

Fyorl added some commits Nov 10, 2012

[feature/autosave] More work on the autosave.
Autosaves should now save per window on pages with posting. When
loading a page, all autosaves of that post will be prompted for,
organised by the time at which they were created.

Added a language string and a few template variables required by
javascript for calculating the time correctly for the user's
timezone.

PHPBB3-11185
@@ -0,0 +1,98 @@
phpbb.autosave_prompt = function (data) {
var dt,

This comment has been minimized.

Copy link
@erikfrerejean

erikfrerejean Nov 14, 2012

Rather use a variable name that describes the variable rather than some acronym.


key = window.location.host + window.location.pathname;
// Extract the f=, t= or p= from the URL and separate with underscores
key += window.location.search.replace(/.*?\W(f|t|p)=(\d)+.*?/g, '_$2');

This comment has been minimized.

Copy link
@cs278

cs278 Nov 14, 2012

Contributor

Would it not be better to activate the autosaving using a data attribute on the textarea? Supplying a unique save key via a data attribute would remove the need to munge the URL and make this function more reusable.

@p

This comment has been minimized.

Copy link
Contributor

commented May 7, 2013

-> 3.2

@EXreaction

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2013

Closing this WIP, feel free to reopen if you want to work on it again.

@EXreaction EXreaction closed this Jul 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.