Per the "discussion" on #2779729: Discuss resurrecting or replace Dreditor to make d.o experimental improvements, this is a solution that was raised to effectively allows us to "save" Dreditor until proper d.o replacements can be implemented #2786361: Move features from Dreditor into drupal.org. It also would allow fix the "FF issue" and effectively allow it to work on mobile devices.

Testing
https://dreditor-drupal.dev.devdrupal.org/node/2776975 (13 MB file)
https://dreditor-drupal.dev.devdrupal.org/node/2759479

Shield credentials : drupal/drupal
drupal.org a/c : user bacon password bacon

Comments

cweagans’s picture

In Firefox, all you have to do is click the link to install Greasemonkey (which is linked from the dreditor project page). It works in Chrome with no additional action. Not sure about IE, but who cares about IE for something like this? I think even Opera supports user scripts without additional plugins.

IMO, I'd rather see it stay as a userscript.

mitchell’s picture

I agree the hurdle isn't too high, and most of us can use and install greasemonkey. I personally don't have any trouble with this, but I understand how many others can. So, this issue is more for them, the people who have trouble configuring their web browsers and also for those who don't know about dreditor in the first place.

I think a better accessibility / usability / availability for Dreditor would be to install it in your own browser if you want to test and develop it. Or if you want, have it drupal_add_js()'d to your user sessions with some configurable option on d.o.

cweagans’s picture

dreditor is for people that want to review patches. If they can't configure their browser, then why in the world are they reviewing patches? What difference does it make if the userscript is served from Drupal.org or from a local directory?

YesCT’s picture

Issue summary: View changes
Issue tags: +Core mentoring on d.o
YesCT’s picture

markcarver’s picture

Title: Add dreditor(s) for users who request it » Move Dreditor into d.o and add a user setting to load it
Assigned: Unassigned » markcarver

This is an amazing solution for a way to "save" Dreditor until proper d.o features can be implemented.

markcarver’s picture

Issue summary: View changes
rachel_norfolk’s picture

I’m really liking this proposed solution - it seems to keep the flexibility and forward momentum of dreditor and drupal.org whilst also supporting the current functionality. Very clever.

Is there anything we can do to make it as smooth as possible ride to get this implemented? I would be so happy to see this for Dublin - I know that’s asking an awful lot...

Oh - and there are far more people making great use of dreditor than those that might be reviewing patches - the ability to very quickly test things and create screenshots via simplytest.me is gold dust for introducing “non coders” to the contribution community. Drupal is better for their input.

markcarver’s picture

#2788239: I want a drupal.org development site for moving Dreditor into d.o is done.

It should be a pretty straight forward port considering all the plugins are already stored in Drupal.behaviors.

I'm hoping to have it working by the end of tonight sometime.

YesCT’s picture

Issue tags: +dreditor feature

grouping it with the other issues about moving dreditor to d.o
(being able to discover it could be seen as a "dreditor feature")
tagging.

markcarver’s picture

Okie dokie.

I was able to get the basic structure for the UI/loading mechanism done.

I've created drupalorg_enhancements (https://www.drupal.org/sandbox/markcarver/2789455) since there's no point in calling it "Dreditor" anymore.

This adds an "Enhancements" secondary tab when on the "edit" tab of a user account:

Keep in mind this is a dev site, doesn't "look" all that great right now, but this is at least a step in the right direction.

As you can see from the above image, I went ahead and ported two simple "enhancements" from Dreditor:

  1. Clone this issue:
  2. Patch filename suggestion:

I was hoping to have all of this done yesterday, but alas client work.

So my new goal is the beginning of next week. I'll likely be working on this quite heavily all this weekend and will spend time making this less "dreditor" and more "d.o enhancements" (including things like theme work).

You can test what I currently have on the dev site:
https://dreditor-drupal.dev.devdrupal.org

Log into:
bacon/bacon

Configuration form:
https://dreditor-drupal.dev.devdrupal.org/user/717216/edit/enhancements

Issue I tested:
https://dreditor-drupal.dev.devdrupal.org/node/2787641

markcarver’s picture

P.S. These enhancements work on mobile too :D

rachel_norfolk’s picture

Liking it so far!

I just started wondering about the name - could we call the option "experimental features", to reflect experimental modules thing we have in D8?

(bacon/bacon not working for me, though)

markcarver’s picture

I actually started out with drupalorg_experiemental as the name. However, I soon realized that an enhancement isn't necessarily "experimental". It could just be a straight up enhancement that is easier/better to do in JS. Like "Clone this issue" given the new tab/window and pre-population workflow. This isn't likely to ever be implemented on the backend in a very effective way.

For enhancements like "Patch Review" (which is likely to be replaced with something better in the future), I think we can add a 'experimental' => TRUE flag its definition. Then we can just indicate which enhancements are experimental and subject to change/be replaced.

Re: bacon/bacon
That's the username/password for the dev site, not the HTTP access (that's drupal/drupal, see https://www.drupal.org/drupalorg/docs/build/development-environments/dev...).

edit: meant to also say that all this verbiage is subject to change and not yet set in stone. So if you see something that could be worded better, please just let me know :D

Mixologic’s picture

Did a cursory look, and man, thanks this is way more robust than what I was suggesting (basically a hack to just drupal_add_js all the javascript with just one checkbox to "enable dreditor features")

This is much, much better as it will be easier to transition things away from being optional to being standard, As well as allow us a way to introduce new js based features without forcing it on everybody (allow for beta, basically).

markcarver’s picture

Ok, so after the convo we had in IRC, I realized that ultimately this API/UI implementation has absolutely nothing to do with drupalorg* projects specifically.

So I decided to split that stuff out into it's own project: https://www.drupal.org/project/user_enhancements

Now it's just a matter of porting the necessary dreditor "features" into the appropriate drupalorg* modules (likely mostly drupalorg_project) using this module's hooks.

markcarver’s picture

Title: Move Dreditor into d.o and add a user setting to load it » Implement User Enhancements on Drupal.org and port Dreditor features
markcarver’s picture

Still needs work, this is just for drupalorg_project with the above implementations using the user_enhancements module.

drumm’s picture

I think clone can live at #2025715: Add "clone" link for issues to make it easier to flesh out meta issues when there are lots of related sub-tasks only. It should be straightforward to fill out that implementation.

I put it next to the “Add child issue” link since that also goes to an issue creation form with some pre-filling. Cloning hopefully isn’t too common, so it doesn’t need a big green button.

markcarver’s picture

FileSize
20.96 KB

@drumm re #19, sure that makes sense. However, until that actually happens (as with all Dreditor features), I'm just going to leave it in (for now).

Here's an updated patch given the work that was done on the user_enhancements module. Will update the dev site shortly.

The majority of the work that I did over the weekend focused on flushing out the user enhancements JS API, allowing for much easier implementations/portability of the code. I'm hoping to have the rest of the features done in the next day or two (client work now).

markcarver’s picture

Dev is now updated. There are two more features that have been ported:

Gábor Hojtsy’s picture

How is this going? Any way people can help? DrupalCon sprints start in less than a week.

rachel_norfolk’s picture

Thanks Gábor for offering.

markcarver’s picture

I've been hard at work, both with my job and this issue.

I started porting the patch review enhancement. However, as I have mentioned before, the code is strewn through out the entire code base. This makes "porting" it (as a singular enhancement, like the ones above) extremely difficult.

I was essentially having to rewrite most of it. So I decided that if that's going to happen, it may as well become a modern/modularized utiltiy.

When people think of Dreditor, the often associate it with patch reviewing. So I'm keeping that aspect of the project alive and and repurposing "Dreditor" from a browser extension to a npm/browser library.

I've started a whole new branch which you can see here:

https://github.com/unicorn-fail/dreditor/tree/2.x

That work has been done for a while, but I'm still working on the completing the new companion dreditor-ui project.

This will allow the patch review enhancement (which is extremely complex) to live elsewhere as a separate project still (for maintainability; I'm sure the will DA appreciate that) while allowing it to be hooked in via the user_enhancements module.

---

I'll be traveling this week, so I'll probably have some more down time than usual and plan on getting something demo-able in the next couple of days.

As of right now, there's really not much anyone can do to help since it's still being worked on. Even though I hope and believe it will be "finished" before the end of this week, I doubt any deployment will happen so close to DC.

That being said, this entire issue is about opt in user enhancements so maybe it can... we'll see.

Gábor Hojtsy’s picture

@markcarver: please keep us updated and let us know if/when we can help. I certainly lack the frontend chops to help with that part but other things I can do to make sure sprinters have some tool to use at DrupalCon that is not going away shortly after -- happy to help.

joachim’s picture

Should #2786361: Move features from Dreditor into drupal.org be closed as a duplicate? that has a ton of child issues too.

markcarver’s picture

The current tool at https://dreditor.org will remain until this issue is deployed. Nothing is "going away", especially for DC.

Re: #2786361: Move features from Dreditor into drupal.org
No, that issue isn't really a dup. It's about making the Dreditor features official (likely backend) implementations on d.o. This issue is, more or less, about a direct JS port (with the exception of the patch review enhancement).

markcarver’s picture

FileSize
65.79 KB
markcarver’s picture

FileSize
144.95 KB
231.03 KB

New user enhancements:

Dreditor 2.0!

As stated above, this is a complete refactor and repurposing from the original "Dreditor browser based extension". All the other little features that Dreditor were originally bundled with it will become d.o specific "user enhancements" (per the patches in this issue).

Dreditor itself is now split into two separate projects that do very specific tasks and only those tasks:

  • dreditor (https://github.com/unicorn-fail/dreditor) - Parse/render a diff and to HTML.
  • dreditor-ui (https://github.com/unicorn-fail/dreditor-ui) - Extends dreditor to display parsed/rendered diff in a UI for the browser.
  • Both are modern, modular and are built using Node + ES6 + Babel + Browserify.
  • Both are event and promise based.
  • Designed to be self-sufficient and work with whatever environment its deployed on.
  • In its current state it's much leaner and more efficient than anything before.
    Minified and gzipped (dreditor.min.js + dreditor-ui.min.js) it only totals 29.64kB!

Combined, these two projects will be d.o's "reviewer" experimental (opt-in) user enhancement:

  • Faster. Much faster! Profiled improvements that range from 10x to, in some cases, 20x faster!
  • Lazy loading/infinite scroll. Helps reduce browser lock-ups, especially with very large files (see 13 MB file example below).
  • UI enhancements to indicate "progress" in areas where it's taking longer due to network latency or above average CPU usage.
  • Overall updated styling, similar GitTower/Gitlab/Github (separated files).
  • Better handling of git-format-patch output, including sequential/multi-patch format.
  • Syntax highlighting, via existing Prism library on site (needs markdown language BTW).
  • Client side routing (node/{nid}/review/{sha1}) for easier sharing of specific patches.
  • Updates browser window/tab title with filename to indicate when in review mode.
  • Mobile friendly-ish (as well as can be expected given how large/long some files/lines can be).

Still to be done:

  • Commenting will become inline and more intuitive (no multi-line selecting, just complicates things). You can already see the circular "+" button when hovering over lines (it currently doesn't do anything when clicked).
  • File list for quicker navigation to files (mostly for extremely large patches).
  • Better handling of query related identifiers with the router (i.e. navigating to specific lines in files).

Known bugs:

  • Manual scrolling isn't perfect, especially mobile touch.
  • Empty footer. This is for file list and buttons when done reviewing (i.e. post comment(s)).
  • Various styling issues (not really important enough to list currently).

Examples (use browser that does not have previous Dreditor installed, incognito mode on Chrome works too):

rachel_norfolk’s picture

Fantastic work!

Looks fab and I do like the new code reviewer - well, I did until I couldn’t add a comment on a line in my Safari 10! Ah - I now read things properly and it is not supposed to do anything yet! :-)

I’ll spend some more time on this over the next couple of days - I’m very excited!

kristiaanvandeneynde’s picture

Looks amazing, nice job!

joachim’s picture

The dev site isn't sending me a login email, but those screenshots look fantastic!

Mixologic’s picture

Dev sites do not have email, login as the user bacon, pw bacon.

joachim’s picture

Thanks!

With the patch review tool, the + in a blue circle that shows in the LH of the patch -- I assume that's how I add a comment, but on Firefox clicking it doesn't do anything.

Also, reloading the URL that the review tool is on (eg https://dreditor-drupal.dev.devdrupal.org/node/2759479/review/6a6b39c0c1...) takes me to a 403 page.

naveenvalecha’s picture

Issue summary: View changes

#34,

With the patch review tool, the + in a blue circle that shows in the LH of the patch -- I assume that's how I add a comment, but on Firefox clicking it doesn't do anything.

This is in progress see #29

Great work @markcaver!
I have given this a shot on https://dreditor-drupal.dev.devdrupal.org It looks fantastic -;)

Few thoughts :
Could we ship with the initial features(parity with dreditor extension) and not include Dreditor 2.0 features for now and do the incremental changes so that peoples will have look and provide their feedback.

markcarver’s picture

Could we ship with the initial features(parity with dreditor extension) and not include Dreditor 2.0 features for now and do the incremental changes so that peoples will have look and provide their feedback.

Suffice it to say: yes, that is what is being done (please read my above comments). This refactoring was always planned (for the various reasons outlined in several issues, both on d.o and GH).

However, due to the coupled nature of the other other "enhancements" (and the fact that it was a browser extension), it made it exponentially more difficult to actually accomplish; if not near impossible. A refactor was inevitable and necessary.

Where some people may be confused in previous issues that I've spoken about this is that the refactoring I was against was for the "browser based extension", which this is not. This is pure JS and much easier to refactor.

Now that this is all modern and modularized, it will be easier to implement feature parity in a systemic and performant way.

With the patch review tool, the + in a blue circle that shows in the LH of the patch -- I assume that's how I add a comment, but on Firefox clicking it doesn't do anything.

Correct. It does not do anything, yet, as I have outlined in #29. There were two primary reasons I added this:

  1. To see how implementing an event listener that adds this element actually impacts the performance of rendering files, which is negligible.
  2. Provide a semi-sneak preview of how I intend to change how commenting works in Dreditor, which is more inline with other tools out on the internet that do the same thing.

---

I spent the better part of the past two weeks (more or less) getting everything to this point. Now that I'm back at work (and no longer traveling), I will have to work on this on my own time again. I expect that I'll have something more feature parity and "finite" towards the end of this next week.

Edit: meant to say towards the end of next week

Wim Leers’s picture

Incredible progress here :) Can't wait to use this!

hestenet’s picture

Issue tags: +Community Initiative

+1 great to see this progress.

joachim’s picture

> Correct. It does not do anything, yet, as I have outlined in #29. There were two primary reasons I added this:

Sorry -- I skim-read your post and leapt straight into trying it.
It's a pet peeve of mine when I post work in progress and say 'foo doesn't work yet' and someone comments to say 'foo doesn't work', so I'm really sorry to have been the one to do that this time! :(

It all looks really slick!!

markcarver’s picture

I'm really sorry to have been the one to do that this time!

It's all good :D

I'm honestly just happy that the feedback so far has been [surprisingly] extremely positive. That hasn't always been the case in recent "community" issues I've been involved with.

markcarver’s picture

and not include Dreditor 2.0 features for now

I just reread my comment in #36 and feel like I didn't adequately explain this portion.

Suffice it to say that these "2.0 features" are really MVPs of a 2.0 refactor.

I always knew going in that when a refactor happened, it would need to be built with these MVPs in mind so it could be properly architected from the ground up.

An example of two of the MPVs and how they affect the overall codebase:

  • lazy loading/infinity scroll
    This is a fundamental shift in how the reviewer in Dreditor operated from it's previous browser based version. Before, the whole file was loaded and then parsed/rendered at the same time. Aside from parsing/rendering huge files that caused DOM lockups, having the entire file directly in the DOM also meant that its refresh/paint rates significantly diminished making thing laggy and in some times, completely unresponsive.
  • syntax highlighting
    We've always wanted to do this, but it wasn't very feasible and would simply bloat the code base. In fact, I probably would have never attempted adding this feature if the PrismJS library wasn't already implemented by d.o. However, since d.o does (now) implement this library I decided that it would make sense to architect the code to take advantage of/tap into this existing feature. This, of course, required some fundamental changes regarding how and when this process happens in the code.

While, yes, there are technically a boatload of new features, they're also simply a necessary change given the amount of "bugs and feedback" we've gotten over the years regarding some serious pain points of Dreditor.

A refactoring is just the best time to do it.

Undoubtedly there will be new features added once it's been released, but these are without a doubt some of the most needed "upgrades" for this to be considered "d.o quality" that is consumed by the masses IMO.

marvil07’s picture

kudos for this great work!

I did tried this on a big patch, e.g. #2759479 on the test site, and it indeed (a) works faster, (b) does not produce a dom lock, and (c) looks great; awesome effort!

andypost’s picture

It works slightly faster in FF and chrome!
Thanx a lot for moving this forward

rachel_norfolk’s picture

Still looking to be going well!

Thinking ahead to when this needs to be implemented on d.o, and the potential size of the patch required, would it now be the time to make this a [meta] issue and have a (small!) series of issues with small patches that do specific jobs? Maybe the first to be the "container" for the extended functionality, another for some of the simpler buttons required (like simplytest.me etc) and another for the code review tool?

It would allow us to get the basics embedded early and see how they are running whilst the code review tool continues to become even more fabulous.

Thoughts?

Wim Leers’s picture

I really can't wait to use this. What can we do to help get this on d.o faster?

markcarver’s picture

would it now be the time to make this a [meta] issue and have a (small!) series of issues with small patches that do specific jobs?

Unfortunately no. This is really an all or nothing improvement.

Once this is implemented, users will have to uninstall the browser based Dreditor extension. If we create smaller issues and only implement a fraction of the features, it would mean that users would have to leave the browser based Dreditor extension enabled (to get the other features).

This would mean that the new and old code would be running together, collide and suck the universe into a black hole. Ok, maybe not that bad, but not something I'd recommend as there would undoubtedly be conflicts that would break "expected" behavior.

It's unfortunate that the code reviewer is bundled with everything else, which is what this is attempting to solve.

What can we do to help get this on d.o faster?

Code/submit PRs to https://github.com/unicorn-fail/dreditor and https://github.com/unicorn-fail/dreditor-ui.

The "Still to be done" and "Known bugs" in #29 is what is left.

As it stands right now, I'm not entirely sure when this will get released. My life has taken a sudden turn of events that has prevented me from actively work on this on my spare time. I can most certainly review any PRs that come though in the mean time.

My new "goal" (as I want to do all of this right) would be by the end of this year, perhaps as a Christmas present to all of us :D

Although, depending on how many people help, perhaps sooner. We'll just have to see.

markcarver’s picture

It should be worth mentioning that you don't actually have to worry about the d.o specific integrations. If you just want to focus on the code itself, then I've created the following gist that will allow you to create a standalone implementation. You'll just have to modify the paths to the libraries appropriately (mine are based on where they're installed on my local d.o dev instance, so I can test both):

https://gist.github.com/markcarver/a0157df7ea8f780ad0eb8a2abc030ea7

markcarver’s picture

Also, you can join me (and others) in the #dreditor IRC channel on freenode if you'd like to discuss things in real time (if I'm available).

markcarver’s picture

I'm hoping to spend some time on this again over the holidays (next few weeks). Anyone have some time to work/test with me on this (as I get stuff out)?

rachel_norfolk’s picture

Feel free to prod me if there's things to test, markcarver. Twitter is best @rachel_norfolk