Opened 14 years ago

Closed 14 years ago

Last modified 12 years ago

#439 closed enhancement (fixed)

[patch] Code refactoring for generic back/forward support

Reported by: jrburke@… Owned by: alex
Priority: high Milestone:
Component: Core Version: 0.2
Keywords: Cc:
Blocked By: Blocking:

Description

I have created a patch to pull the back/forward support out of BrowserIO.js and into its own package. This was done to allow generic back/forward support for applications even if they do not use XMLHTTPRequest to define "page transitions".

I moved the back/forward code out of src/io/BrowserIO.js to a new file, src/nav.js. dojo.nav.addToHistory() is the main entry point to the back/forward support. I also moved dojo.io.setIFrameSrc() dojo.nav.setIFrameSrc(), since it was needed by addToHistory(), and it seemed to belong in a "nav" package. As a result, BrowserIO.js, IframeIO.js and RepubsubIO.js were modified to add a require("dojo.nav"); line.

The code moved from BrowserIO.js to nav.js is mostly the same, although I did change how the code sets up dealing with back setup to mirror the same setup that was used for forward.

I tested with MSIE 6.0, Firefox 1.5 and Safari 2.0.3. There is a test page at tests/test_nav.html. It mentions how to use dojo.nav, as well as important caveats for MSIE and Safari. I'm not sure if I need to do anything to integrate with the automated tests, but I'm not sure this test can be automated since it needs to run in a browser and the browser back/forward buttons need to be pressed.

I noticed that by default, preventBackButtonFix is set to true in bootstrap1.js, which means that by default, dojo.nav won't work in MSIE unless the developer sets preventBackButtonFix to false in their djConfig. I am curious to know why it is set up in that way.

I created the patch using TortoiseSVN's Create Patch command. My source snapshot was last updated around 12:45PM PST, Feb 13, 2006. My CLA is on file with the Dojo Foundation. If you would like any changes to the approach above, I'm happy to make them and resubmit the patch.

The patch can be retrieved from here:

http://tagneto.org/dojo/patches/backforward1.patch.txt

I put up my dojo snapshot and you can try the test page here:

http://tagneto.org/dojo/snapshot/tests/test_nav.html

James Burke

Attachments (3)

backforward1.patch.txt (39.3 KB) - added by jrburke@… 14 years ago.
Patch file.
backforward2.patch.txt (37.9 KB) - added by jrburke@… 14 years ago.
patch file that puts back support in dojo.undo.browser
backforward3.patch.txt (34.6 KB) - added by jrburke@… 14 years ago.
Patch with tabs instead of 4 spaces

Download all attachments as: .zip

Change History (12)

Changed 14 years ago by jrburke@…

Attachment: backforward1.patch.txt added

Patch file.

comment:1 Changed 14 years ago by alex

Component: GeneralCore
Milestone: 0.3release
Owner: changed from anonymous to alex
Priority: normalhigh
Status: newassigned
Summary: Code refactoring for generic back/forward support[patch] Code refactoring for generic back/forward support
Version: 0.30.2

comment:2 Changed 14 years ago by alex

Hey James,

This is great stuff. One thing I wonder about is whether or not we should be putting the forward and back button stuff into a dojo.undo.* and not a dojo.nav.* namespace? What do you think?

comment:3 Changed 14 years ago by jrburke@…

dojo.undo could work too. I'm a little unsure about picking up the extra code weight with dojo.undo, and want to sort out in my head the more philosophical issue of what does back/forward in a browser mean: is it back in navigation or back in functionality/changes? DHTML apps may blur this line, even though for the browser I think it is strictly back in navigation (does not necessarily mean the functionality or changes you just did were undone).

But I can see where the dojo code is really providing the developer the option of either. They get to define what back means, not the browser.

OK, if you like, do you want me to redo the patch to move it to dojo.undo? I'm happy to do so, but if you already have the changes scoped out, then that is fine too. I'm also working on ticket 379, but I would like to get this patch sorted out first.

If I'm doing the dojo.undo work, it seems like the back/forward case is a specialization of the generic undo, does that seem right? Convert the back/forward case to use the generic undo code: use the generic undo code as the holder for the back/forward lists. Still seems like it would be nice to have a static method to adding to the history stack. Any thoughts? dojo.undo.browserHistory.add()? Any other implementation notes you have would be appreciated (if you aren't going to do the work).

Along with that change, what if we also do the following:

  • move dojo.nav.setIFrameSrc back into the dojo.io package, but put it in the top package (io.js) instead of BrowserIO.js. That way IframeIO.js and RepubsubIO.js won't depend on dojo.undo, but dojo.undo.browserHistory (or whatever the name) will depend on dojo.io. I had also considered putting setIFrameSrc up in the dojo.html package, but that seemed like a lot to pull in for the io package.
  • in XMLHTTPTransport.bind(), the first statement block automatically adds the kwArgs to addToHistory() if no URL and no formNode but the back properties are defined. Add a "deprecated" statement to that block (or can we just remove it -- maybe too soon for that)?
  • Move the other case of adding to history stack from XMLHTTPTransport.bind() to the more generic dojo.io.bind() method. This way all transports can take advantage of the back support. It might not make much sense for iframe and repubsub, but I'm hoping to add a ScriptSrcIO (ticket 379), and I didn't want to have to replicate the behavior in there. I would think that if you are using iframe or repubsub you may not be registering back support anyway. But to be sure, we could add something like a supportsBack() method to the transport interface, and if the transport supports it, then dojo.io will do the backforward registration. I like it up in dojo.io.bind() too so that later if we can ever work out a conditional include/compile thing where maybe dojo.undo.browserHistory is not loaded, then dojo.io.bind() is smart enough not to call addToHistory. In fact, perhaps that is what the preventBackButtonFix flag does: if that is set to false, then somebody (bootstrap1.js? not familiar with that yet), does the dojo.require() for the back/forward support. That way dojo.io does not directly depend on back support.

comment:4 Changed 14 years ago by jrburke@…

After looking around a bite more, I think I will redo the patch with just the following changes for now (if it sounds OK?):

  • Move dojo.nav to dojo.undo.browser. Keep the implementation the same for now (don't try to use dojo.undo.Manager). It looks like it will be a bit of work to use dojo.undo.Manager, and I'm not sure it would work out.
  • Move dojo.nav.setIFrameSrc() to dojo.io.setIFrameSrc(), but put it in src/io.js, not in BrowserIO.js. That will remove the require statements from IframeIO.js and RepubsubIO.js (keep them the same as they are in SVN now). BrowserIO.js will require dojo.undo.browser. dojo.undo.browser will depend on dojo.io, since it needs dojo.io.setIFrameSrc().
  • Put a "deprecated" message in for this case: in XMLHTTPTransport.bind(), the first statement block automatically adds the kwArgs to addToHistory() if no URL and no formNode but the back properties are defined. This seemed like a shortcut to get to the back functionality without actually doing a dojo.io action.

comment:5 Changed 14 years ago by jrburke@…

I made the changes described in the last post (dated 02/18/2006). I'm happy to make any changes necessary if this patch isn't right.

The patch can be retrieved from here (I will also attach it to this ticket):

http://tagneto.org/dojo/patches/backforward2.patch.txt

I put up my dojo snapshot and you can try the test page here:

http://tagneto.org/dojo/snapshot/tests/undo/test_browser.html

James Burke

Changed 14 years ago by jrburke@…

Attachment: backforward2.patch.txt added

patch file that puts back support in dojo.undo.browser

comment:6 Changed 14 years ago by alex

hey James,

This patch looks good. If you can ensure that tabs are used for indentation and not spaces, I'll be happy to apply it.

Thanks so much for your hard work on helping us clean this up.

Regards

comment:7 Changed 14 years ago by jrburke@…

Duh. Sorry, I read the style guide wrong. I thought it seemed a little weird. backforward3.patch.txt has tabs instead of 4 spaces.

The patch can be retrieved from here (I will also attach it to this ticket):

http://tagneto.org/dojo/patches/backforward3.patch.txt

I also made a change to the test page, test_browser.html: I commented out debugAtAllCosts: true and dojo.hostenv.writeIncludes().

Also, you may want to double-check src/undo/package.js. I think I did it right (it worked when I used dojo.undo.* in a test page, both for dojo.undo.Manager and dojo.undo.browser), but I'm not sure if it is correct for JS environments beyond the browser.

James

Changed 14 years ago by jrburke@…

Attachment: backforward3.patch.txt added

Patch with tabs instead of 4 spaces

comment:8 Changed 14 years ago by alex

Resolution: fixed
Status: assignedclosed

latest patch merged as rev [3198]

comment:9 Changed 12 years ago by (none)

Milestone: 0.3release

Milestone 0.3release deleted

Note: See TracTickets for help on using tickets.