Question: would efforts going towards the decoupling the structure pattern from folder_contents be welcomed?

[metatoaster]

The structure pattern looks quite useful for any kind of file/folder-like resources, however it makes assumptions on certain things, such as the availability of the paste button (which can be unavailable if the underlying view returns a json string without that button defined), and other Plone "folder_contents view"-centric ideas that make presenting a proxy file/folder view of non-Plone local resources (such as a view of some folder on the server, proxy view of git/mercurial repositories) rather difficult, and customization of action buttons isn't fully possible (i.e. paste button must be available even though the specific view implementation may not provide one). I mostly want to know if this is within the scope of what plone mockup is wanting to achieve, so that no time is wasted on irrelevant goals.

[djay]

I’d like to see the action buttons fully configurable like the only folder_contents was. ie coming from folder_buttons actions. At the moment its not possible to add your custom actions in there or remove any. Might be tricky to get the UI right to make it look ok with more actions but it should be possible.

[vangheem]

@metatoaster Yes, we would welcome that. There was a branch a while ago that attempted to do that so we could reuse it to manage users and groups. I don't know what happened to it.

@djay yes, you can add custom buttons. See how workflow action is implemented for instance: https://github.com/plone/plone.app.content/blob/master/plone/app/content/browser/contents/workflow.py

[metatoaster]

At least not overridable currently as how it is done now within folder_contents. What I am trying to do is to create a completely new view that make use of structure to render some file/folder-like information sourced from a location external to the Plone site. The view currently is loosely based on the FolderContentsView found in plone.app.contents, where the get_actions method is overridden, currently only returns a dummy field:

    def get_actions(self):
        return [{u'url': u'foo', u'title': u'Foo', u'id': u'foo', u'icon': u''}]

As is, clicking on the pagination button (or any interaction) as is will simply cause this be raised:

TypeError: self.buttons.get(...) is undefined app.js:209:9

Since app.js currently is hard-coded to assume that button exist, which in this case does not. Naturally, providing a new action with the ...u'id': u'paste'... will alleviate the issue. First cut of the fix will have to check that button actually exists before trying to manipulate it, then later figure out how to include button-specific availability conditions to the IStructureAction implementations into the script.

[metatoaster]

So far I have gotten these things done (off the 2.0.12 tag, because I can't use it with 2.1.x due to the radical naming changes that renders mockup incompatible with Plone 5.0):

• adding test cases to cover existing default behaviour where changes were made (more comprehensive than before, but not nearly enough for something complicated like this)
• script no longer throws an exception if paste button was not available [changeset]
  • allow the option to render no action buttons (only button available is the selection button)
• full control over what columns were visible (previously default list merging behaviour was used) [changeset]
• allow removal of as much of the default content as possible (possible to have no portal_type provided by data as the computation for the css value is moved from template to the javascript method) [changeset]
• saved column is no longer shared across all views (it was specified by a single hard-coded cookie key/value) [changeset]

These changes, along with a JSON response view that returns information similar to what plone.app.content.browser.vocabulary.VocabularyView returns, more or less allowed the implementation of a new folder_contents like view using a custom data source.

However, there are still function and attributes remaining that I can't find out how to override, such as the queryHelper which is quite tightly coupled to the VocabularyView, and that data-keys that are directly tied to indexes that is part of the default portal_catalog. i.e. review_state, portal_type which are not necessarily relevant to user and groups (see mockup/patterns/structure/js/views/tablerow.js). Also, to dynamically load the next set of contents (for a subdirectory, for instance), there isn't a way to easily poke at the script to get it to generate the correct link/query/url to the data source for that, there are so many layers of indirection in this javascript library for that particular functionality (through the QueryHelper and various backbone classes).

At this stage I am not sure how to untangle that particular functionality. I supposed I got more tests done and made more options customizable and it can behave correctly, but to get the dynamically in-place loading feature is simply impossible (or require more convoluted overriding) for anything outside of the current behaviour.

Please do have a look at that particular branch I have started and give comments/critiques. There are still some bugs that have cropped up from not providing some of the keys that each row in structure needs (one result is that the select all button does not behave correctly when deselecting), so there are still quite a few things to work out.

[gforcada]

@metatoaster any reason to work on your fork instead of directly on mockup repository? But anyway a pull request even though it can be WIP is always easier to share and show code.

[vangheem]

I don't understand the problem with 2.1.x. It is currently used in Plone 5--there is no radically different naming. The only thing notable is "mockup-patterns-base" is deprecated for "pat-base".

Your changes look fine.

For customizing the querying, now of course, nothing is impossible :). You'll want to look at collections/result.js and it's use. The queryParser function is generating the query params for the ajax request. What you need to do is be able to provide alternate parsers. Then you just create a view that responds to the requests.

[metatoaster]

@gforcada This was due to the fact that I had started working on this while I was waiting on the clearance of the contributions agreement (and was not in the Plone Foundation group).

@vangheem The problem is that if I used the master/2.1.x under the default Plone 5.0 install (using mr.developer for just this package plus my other development ones), the web browser's js engine complains that it can't load ${portal_url}/pat-base.js and then requirejs giving this error:

Uncaught Error: Script error for: pat-base
http://requirejs.org/docs/errors.html#scripterror

I guess this might be fixed with a configuration issue, but I am unsure where to poke and at the time I started this I only wanted it to work. I had to struggle a bit to get mockup's tests properly running, dealing with bower/npm not exactly working as I thought it would have at the time but eventually I settled on a working config I stuck with it, so I never investigated further why that didn't work.

Anyway, I have rebased my current development branch directly on top of master under a new branch and pushed this to plone/metatoaster-structure-master, and with that I guess I can do a pull request as requested.

My question is actually more how to specify/override/remove the use of QueryHelper and manipulate the base vocabularyUrl for specific links (since certain implementation it doesn't make sense to use these portal_catalog specific things).

After reading the code some more (and been thinking about this on and off since I got the updated comments in) I think might be able to come up with something; it might be the most drastic change I am about to do.

[vangheem]

You probably just need to run an upgrade or create a new site right?

Can't you just implement a different view that you replace vocabularyUrl with?

[metatoaster]

You probably just need to run an upgrade or create a new site right?

I am working with the current version of the Plone 5.0, with the resource registry configured to have development mode enabled and the develop options enabled.

In a clean site with the development mode disabled, trying to build the bundle results in the same error.

Traced this down further: basically the relevant registry entries for pat-base aren't defined, so the require.js defaults to the root of the (as I reported, ${portal_url}/pat-base.js, example: http://localhost:8080/Plone/pat-base.js) where that's not the valid location (valid location is ${portal_url}/++plone++static/components/patternslib/src/core/base.js), This is fixed by adding this registry entry:

  <records prefix="plone.resources/pat-base"
            interface='Products.CMFPlone.interfaces.IResourceRegistry'>
      <value key="js">++plone++static/components/patternslib/src/core/base.js</value>
  </records>

This probably should be added to Products/CMFPlone/profiles/dependencies/registry.xml in Products.CMFPlone and the relevant upgrade step for when this is deployed. Again, development must be enabled (or select the build option) for the plone and/or plone-logged-in bundles to see what the lack of the above record will result.

After adding that particular registry record, the error is gone but only the default view is rendered for the structure pattern (even for the default folder_contents view). No idea why, no error messages anywhere, it just appears that it's not picking up from the options correctly (hence the default view), I guess?

Can't you just implement a different view that you replace vocabularyUrl with?

This requires having a view that speaks exactly like VocabularyView and the catalog parsing. Basically I don't want to deal with query={"criteria":[{"i":"path","o":"plone.app.querystring.operation.string.path","v":"some_nested_resource/src::1"}] and friends when only some_nested_resource/src means something to my application (and given that I have a view that implements IPublishTraverse I want the action to be able to construct my own URLs based on that view (e.g. ${context}/@@storage_contents/some_nested_resource/src, @@ added for emphasis that it's a view) for the given context with that particular subpath (e.g. ${context}/@@structure_json/some_nested_resource/src) to fetch the JSON containing the items). Of course there is also paging to consider, and making this compatible with traverse subpath views (obstacles include no way to configure how to do history.pushState as it must end with a view which is why it won't pushState properly with a view that implements IPublishTraverse). This part especially got me to feel that this particular pattern is quite tightly coupled and of limited cohesion (for instance, at the link level why not simply specify using data attributes the url to pushState with the data source with associated data to populate the structure with? I guess reason is that path calculation done on the client side alleviate server load, but then I would argue that this whole pattern stinks because it stops clients without javascript from seeing a single file, though I am digressing now).

Between trying to work out how to get this to do what I want and trying to get the 2.1.x branch working with Plone 5.0 has been rather frustrating at the moment, but getting this sorted out has got me onto path towards progress, I guess that counts as something.

[vangheem]

Ahh, you need to use master of CMFPlone.

[metatoaster]

Well, turns out I was wrong about the pushState thing, just that there were a lot of other things piled on top/around it blinded me a bit (by the queryHelper/queryParser and the things it generates). That part is simply specifying the base as the base_url + '/traversable_view' and leave the appended as empty. Would have preferred the {path} token replacement that is presented by the contextInfoUrl parameter (which fed into getAjaxUrl) which would have been a bit more clear as to what it does. At some point what all of the arguments mean will need to be documented and be changed later if needed be.

Anyway, this has taken a lot longer than I had wanted, but I think most of the nasty bits have been untangled and overrideable. There were some pretty drastic changes, however I have added a number of new tests to address this. Places I want some comments on are in changeset d35479f where I moved the location of the constructors for queryParser and queryHelper to within ResultCollection and rejiggled everything around so that it is possible to override everything at once (well, not that it was possible to previously). This is done by overriding the entire ResultCollection implementation, specified via an already loaded requirejs module. This probably is a bit drastic, as there may be further content types (say for the representation of all Plone users) be presented by the portal catalog/vocabulary view and it would be nice to not having to reimplement this completely for that (should be still be possible). However for custom types it does make sense for it to have its own ResultCollection implementation. Following up with this is changeset fdd5aef where I follow up with a test that show the AppView.ResultCollection.QueryHelper instance functioning correctly with the default instantiation arguments with the default ResultCollection.

There are still bits that I want to improve as I am not quite done yet (such as the assignment of the /view suffix on the links is currently hardcoded into the template which may be unwanted), but at least the hurdle of not being able to see or control how the paths represented currently by the structure pattern and its usage (set/get) is removed and that it's now possible to largely override nearly every single part of the structure pattern to do what a user might need for a custom view that resembles the default folder_contents view for representation their list of/within custom content types.

Lastly, I hope my requirejs usage is kosher. It is working currently with my custom implementation of the ResultCollection and navigation usage.

Unrelated to what I want to do, but related to how this pattern really should work, is that there is no Javascript optional view of folder_contents. The structure patterns should be able to poke into an existing html table that a folder_contents type view might generate and modify the things as it see fit (i.e. get pagination working, present date/time as how long ago, etc).

Would be keen to know if what I've done so far is on the right track for inclusion into master.

[vangheem]

I apologize it took so long for me to review. This got buried.

Might it be easier if you thought of this as a something that could be overridden and extended with sub-patterns instead of trying to make everything configurable? For instance, you could avoid the whole use of collectionConstructor option with require and instead just have a sub-class that overrides an attribute.

Otherwise, it seems okay to move all that into the ResultCollection class. It's a bit more mis-direction but there is already a lot of that going on...

[metatoaster]

I am not familiar with sub-patterns since I didn't look at every single pattern to find an example on how this can be created. While it looks like I am trying to make everything configurable, I prefer to see it as providing hooks for integrators to be able to fully control every aspect of this pattern by providing a clear way to put their code into the system. Also, this is an attempt to make this somewhat more modularized/pluggable by custom modules.

Perhaps I should list some reasons and illustrate why I ended up making use of require for this. Reasons include:

• ResultCollection is used from within AppView which is in turn is created by the structure pattern; as far as I know the top level pattern is the only piece that interfaces with the html (i.e. the entry point) that integrators can manipulate or have control over. If sub-pattern can somehow override the ResultCollection to be my custom version that doesn't require the tightly coupled requirements of queryParser/queryHelper I would consider making use of sub-pattern.
  • Also the navigated path here was acquired/manipulated in multiple locations that poke into the queryHelper object through the AppView object, or even inconsistent referencing to queryHelper within AppView itself. Notwithstanding that, the point was that the code that set the inner path have to go through queryHelper which contains a lot of baggage that an integrator might not want to deal with (I certainly didn't). A clear approach to set/get the current path in this case (i.e. via relevant methods) clarifies the intent of what is being done while providing a clear way to override if the integrator intends to subclass this whole thing.
• As mentioned, I wanted a Backbone.Paginator.requestPager subclass (which ResultCollection is one of) that didn't rely on the queryParser/queryHelper for generation of the server_api flags to interface with the default VocabularyView (in my case, I provide a view that implements IPublishTraverse to generate a more natural URL to the target path). As written, it seems impossible for integrators to override the entire thing without requirejs.undef the library mockup-patterns-structure-url/js/collections/result and loading their own module in its place is complicate and convoluted (it's like monkey patching just a given view within a page and if the integrator loads this code to the global level bad things will happen). Also this doesn't actually work because queryHelper was on the AppView level and it pokes into it for the path which then this custom requestPager will have to talk to. See above point again.
  • This leads to why I moved all queryParser and queryHelper into the default ResultCollection class; I see that the functionalities offered and how the queryHelper is integrated into this was very tightly coupled, so might as well put them together into one class. It really is a thing that do one thing well which is to generate the urls to access the JSON on the server.
  • I personally felt there was no benefit splitting them up as implementing compatible but custom queryParser and queryHelper on top of a custom Backbone.Paginator.requestPager really convolutes everything - just make a custom requestPager and be done with it if needed. Also, the scattered nature of this code made it more difficult to follow (as opposed to defined modules/plug-ins that poke into other files, as this demonstrates clear intent and point of modularity).
• A clear way (through require) to override the collection class means that the integrators are not forced to subclass and register a new subclass of the structure pattern for any potentially trivial override, all they have to do is to register their custom code via define, specify them to load using the resource registry configuration and it will just work (at least it did for me, see end of post).
• What about per-item customization of action menu options? I can see how I can achieve this (and it's on my next thing to do to finalize what I really wanted to do) if I can pass the model for the current row to a custom method that can be specified via a similar require hook to return the intended results for rendering. Right now this is defined at the pattern level, though the individual triggers are of each actions are already done through require already (see the default definition, demonstrated by this test description and the actual test).

Only major concern as I see it is that the library needs to be preloaded through the async loading before the pattern is loaded to allow the custom stuff to flow synchronously like it did previously. Though the requirement and framework to make this happen is already in place through the resource registry. As you also mentioned configurability might also be a drawback, but modularizing through require made sense to me as it is a well defined way for the modularization of javascript code.

Lastly, while working on mockup I also work as the integrator to achieve what needed to be done. I am trying to produce a file browsing view that views a file/folder like structure (e.g. such as from a git repo) and I found that doing a completely custom requestPager subclass was much easier to achieve what I want to do because I can ignore all the bits that deal with the Plone Catalog (as the implementation of my custom view doesn't even talk to that). As an example, I have deployed a demo Plone 5 instance that contains my above code, with a live demonstration provided by this view. It's basically a viewer to the listing of blobs within a git repository (While the navigation to folders will work, navigation to files will reload the view and show nothing as I haven't implemented the handling for that yet).

I do have to apologize for another long rant about/against this pattern but please note that I really want to make this better and easy enough for integrators to use without them tearing out hair like I've done so for the past month while working on this (plus I had to learn all sorts of stuff related to doing actual javascript development, getting various stuff running and I probably should document all of this if I have time, but all this can be considered as a beneficial lesson). Also apologies for this large number of changesets and changes in general for the pull request (and it isn't quite over yet, if this needs breaking up into per feature I can try but it will be difficult due to having needing various fixes to actually be able to provide the new features). Though I still hope that these will make it in because so far it is working well so far.

[vangheem]

I'm fine with you separating the collection object and I see why you did it, I'm just suggesting that doing it in configuration this way might not be the best way. If you want significantly change the behavior of something, it seems better to me to do it as a new pattern, sub-classing and overriding. Yes, not everything can be achieved right now by simply overriding but those are the bits that you are refactoring :)

In any case, I might end up being okay with using the require call inside the pattern initialization. It just seemed a bit odd to me at first look.

You do have a lot of commits, can you squash them into one before you're ready for a final review?

[metatoaster]

For the squash you mean the process outlined in this Squashing Commit section of the dev documentation right? If so roger, makes sense because that's generally my workflow also. I was initially confused because I thought squashing them into one commit, but I guess it does make things easier. Though I do want to confirm that the commits I've done in this branch are ultimately added to master as relevantly so, right?

[vangheem]

Yes, here is more info on it: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

[gforcada]

There's also http://docs.plone.org/develop/coredev/docs/git.html for the sake of it (although a bit more basic maybe)

[metatoaster]

Looking at my changes so far, I do see the benefit of squashing down some of the commits to aid in review, but definitely not all the way down to one commit as that would remove some of the story and destroy the atomicity that I try to maintain when developing this feature as the changes were incremental for the most part.

My preference is of course keeping the commits as is after the rebase on master, and with the review done on the one giant (~2500 lines change) commit as the option if this is the preference, and once the review is done the rebased version (which I have been pushing) is just accepted. Again, mostly I want this done is so if something breaks with these changes the context of the changes that may have caused that hypothetical problem can be traced down to that one commit rather than the giant 2500 line long commit.

Of course, some of the changes to the testing structure I could have split up from the later commits and squash down into where I introduced the tests in the first place. I am just really hesitant to slap all those changes into one commit, is my biggest concern.

In any case, git rebase on master was done with a couple fluff/fixup commits broken up and merge down to where they should have been applicable. I can of course split up #618 down to individual feature/bugfix "patches" per topic to aid with review, if needed be.

[vangheem]

I understand where you're coming from with being able to revert to a previous state of your work. Maybe make another branch from the branch you're working on and squash to one commit there so you can keep your history on the working branch. Once on master, I'm not sure it's useful to keep a lot of extraneous commits. Right on, you have 39 commits; something like that is really annoying for git history when it's preferred to have 1 commit for each PR/change.

Also, keep in mind, running rebase on master can be very annoying with many commits.

[metatoaster]

With the extra amount of history it is hugely useful for say git-bisect as I've used this while building this to figure out what went wrong after I rebased. Another useful case is if users later develop issues on the development branch it would be great if they could git blame on the line that failed and see the context of the changes which may give them clues to deal with the issue. Also, having run git rebase it is much easier for me to deal with smaller atomic changes one at a time rather than a giant 2k line commit which I generally have more difficulties with in my experience. Then again, each to their own.

In any case, I am not here to change policies but really want to clarify the application of this policy and figure out why this might be a choice.

(I am mostly miffed that the time I spent working on atomic commits with clear commit messages per change is essentially wasted).

[thet]

@metatoaster @vangheem what is the state here? Just missing the final squash?
I'm about to also work on the structure pattern and it would be helpful, if this PR would be merged first.

[thet]

@vangheem regarding inline require calls - is requirejs be able to pick up these pattern-option declared and only run-time known dependencies when building a bundle?
I suspect that - although I wish it would, as it would give us a nice plugin mechanism.

[metatoaster]

@thet Honestly I really would prefer those changesets remain as is because I've made them quite atomic - please go through them and you will find that most of them describe a key change/fix to make things happen and tells a story on how the changes come about. Squashing them will just render all that work destroyed and also I find it difficult working with giant diffs - they are practically useless in trying to find issues later after the fact (i.e. with git blame - no single commit message related to the change to refer to), especially given the complexity involved with this particular pattern.

[metatoaster]

regarding inline require calls - is requirejs be able to pick up these pattern-option declared and only run-time known dependencies when building a bundle?
I suspect that - although I wish it would, as it would give us a nice plugin mechanism.

As long as the requirejs define calls are loaded (via a compiled bundle or not) before the pattern at hand is invoked (which should be - as that latter code should only be started as body.ready is fired), passing the defined name into requirejs using var mod = require(name) will assign the module (or whatever defined) to mod in this example.

Can see an example of this in this test. Note that it needs to be preloaded first asynchronously before it can be used as is within the code, which the existing framework can handle without issues.

[thet]

@metatoaster I understand your standpoint about your atomic diffs. Although I think there is some potential of squashing related commits - e.g. actionmenu test and then split and then further improving it could just land in one commit, IMO. I'd prefer fewer commits, but this is not a hard requirement for me.

Regarding the inline require js calls. As the depenendencies which are loaded inline (defined here: https://github.com/plone/mockup/pull/618/files#diff-cc6e4653be0bf890095cdc16bd20134aR7 - loaded here: https://github.com/plone/mockup/pull/618/files#diff-2bf36157ca99baaf4736cc9eb33adcc3R36 )
are also defined in the modules define call (here: https://github.com/plone/mockup/pull/618/files#diff-2bf36157ca99baaf4736cc9eb33adcc3R8 ), there should be no problem.

The PR looks good although I think it's adding more code coplexity.
Is the functionality finished or is still missing something from what you wanted to achieve?

Let's wait for @vangheem statement...

[vangheem]

If @metatoaster is ready for a final review, we can look at getting it merged.

Currently, the PR has 40 commits. While I don't see any obvious clean up commits, you did change direction on how to solve problems. rebasing could potentially be hell if it's not merged or squashed.

@thet if you're planning on working on structure, can you also give it a review and give your thoughts?

[thet]

... doing.

[thet]

Reviewed.

@metatoaster , no obvious problems, much is just refactoring of existend code, much is improvement. I saw some problems (like the assumption that a default page must be non-folderish, which is not), but these already existed in the previous code - so their scope is not part of this PR.

Also, there are no backwards incompatible changes, are there? Looks like the changes in pattern options are also backwards compatible.

The changes make a lot sense all together and I don't see much value in having atomic commits so I would also really squash the commits down to one. No one will analyze each commit one by one. If you had 5 commits - each for another feature, we wouldn't ask you to squash them.

But you have a lot of good commit messages - they should go into CHANGES.rst (probably a bit rewritten).

In short, what's missing:

• CHANGES.rst update with a summary of what has changed (from your commit messages).
• A commit-squash.

Then I'm + 1 for merging.

[vangheem]

@thet Thanks for merging.

@metatoaster I understand you don't want to lose all your commit messages. Keep in mind, you can combine those separate commit messages into one large one to push for this commit.

[thet]

@vangheem did not merge yet ;)

[metatoaster]

@vangheem Not so much the message but more the message with the changes at hand.

@thet There shouldn't be any backwards breaking changes, but if there were I've done git-bisect to narrow down the specific change that broke my behavior before, hence I am very hesitant in just squashing all of them.

As a compromise, I am quite happy to rebase/squash all test only commits and trivial changes down to whatever previous code changes as that does make sense, but if that isn't satisfactory I will be less happy and squash them all down to one.

Also, I've done rebase on master multiple times with this many commits, there hasn't been that many issues to be perfectly honest.

[thet]

+1 for the compromise. Let's see how it looks after that.

Don't forget the changelog entry!

Loosely following the semver.org suggestions of version numbering, we should raise the mockup version to 2.2. Right?

[metatoaster]

Thanks for that.

I ended up doing a lot of rejuggling of the changesets but I am quite used to doing this since I am the one being pedantic about this, the results ended up being mostly satisfactory (somewhat dissatisfied with how opaque the change to ActionMenu was done (as I really hate changing code while moving files around), all things considered it isn't that big of a change). End result is that there is one changeset per feature introduced, and on the plus side I rewrote the history in such a way that the pushStateUrl parameter I introduced as an alternative to the urlStructure parameter is now completely decoupled from the traverse subview compatible pushState support - again things like this is why I am insisting changesets be done the way it did.

Changelog entry file has an entry roughly correlating to what I've done. The actual changelogs per commit is still long since I haven't not rewritten them but I did prune off entries that became irrelevant. for the most part. Changes roughly follows this

• Code cleanup for flexibility in supporting third-party users and also the changes that follow.
• Complete revamping of the action menu and decoupling of items from its callable.
• Detanglement of the ResultCollection and its interaction with utils.QueryHelper and relocated its queryParser so that getCurrentPath / setCurrentPath can be consistently used/implemented by other subclasses of Backbone.Paginator.requestPager.
• Possibly controversial change - support for a special viewURL argument in place of getURL for items returned for pre-calculated view urls for the item (as some items will not need /view).
• Support for IPublishTraverse usages for this pattern, ensuring the pushState urls can be correctly calculated for this
• Finally, the alternative pushStateUrl format parameter to urlStructure.

I had planned to also implement the requirejs defined button/action similar to the per-item action item, mostly to make uniform the implementation for both of these things, but I haven't got around to doing that yet.

I also would like to see the breadcrumbs in here to be its own thing, but that feels too much of a scope creep given the state of things.

As usual, please review and let me know what else may be needed to get this merged.

Oh yeah, lastly - git rebase was done with the tests done on all the commits, to aid in future bisects, if needed.

[metatoaster]

Closed via #618