Support from Acquia helps fund testing for Drupal Acquia logo

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 drupal.org
YesCT’s picture

markhalliwell’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 » markhalliwell

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

markhalliwell’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.

markhalliwell’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.

markhalliwell’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

markhalliwell’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)

markhalliwell’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).

markhalliwell’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.

markhalliwell’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
markhalliwell’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.

markhalliwell’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).

markhalliwell’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.

markhalliwell’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.

markhalliwell’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).

markhalliwell’s picture

FileSize
65.79 KB
markhalliwell’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.

markhalliwell’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!!

markhalliwell’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.

markhalliwell’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?

markhalliwell’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.

markhalliwell’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

markhalliwell’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).

markhalliwell’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

kristiaanvandeneynde’s picture

Did this go live somehow? I am seeing my dreditor buttons again and I'm still using Firefox.

markhalliwell’s picture

No. I haven't had much time to work on this as I had hoped. Perhaps during DC next week, I'd like to collaborate with others if anyone is interested in this and going to be there.

My guess is that perhaps FF rolled back some of their security because too many extensions developers were complaining that users couldn't use their extensions? ¯\_(ツ)_/¯ idk

alar’s picture

I'm here because https://www.drupal.org/patch/review has a link to 'or use Dreditor'. And that leads to

We're saying goodbye ;-(

Dreditor as you know it (a browser based extension) is in the process of being decommissioned.

Before we send folks scurrying. What is the forecast for Dreditor?

markhalliwell’s picture

What is the forecast for Dreditor?

Cloudy with a chance of flash flooding.

---

I determined at DrupalCon that absolute MVP needed to get Dreditor 2.0 out the door will be adding the commenting ability. Everything else could be addressed or added in the future.

That being said, @hestenet raised a valid point in #2490332-207: Evaluate whether to replace drupal IRC channels with another communication medium and at DrupalCon:

Drupal.org may be adopting something like Gitlab in the near future. That would, of course, ultimately make this endeavor completely obsolete (the patch reviewing aspect anyway).

It also appears that no one is or has been willing to help me actually work on the code. That has been the biggest hindrance because it puts all the responsibility and dependency squarely on my shoulders. I have not had the time to work on this as much as I would like.

So, unless more people can help me with this, I suspect this is not going anywhere anytime soon.

markhalliwell’s picture

And by "help" I mean actually getting involved with the code/PRs

markhalliwell’s picture

This would mean that the new and old code would be running together, collide and suck the universe into a black hole.

My previous statement regarding separating out the patch review from Dreditor's other features is no longer applicable now that #2918456: Friendly URLs for Drupal.org issues was implemented. This change effectively broke nearly all features that previously relied on the simpler https://www.drupal.org/node/123456 URL format.

Thus, I suppose the patch review aspect of the features is now holding up this issue and can be moved to an entirely new separate issue allowing the other features to move forwards in this one.

markhalliwell’s picture

Patch review can be done in #2788179: Implement "patchr" as a d.o user enhancement.

I've also requested that #2928568: I want a drupal.org development site for Dreditor be rebuilt so I can move forward with this issue which can add many of the smaller features that were broke.

markhalliwell’s picture

I'll try to make some significant progress on this again this coming weekend.

hestenet’s picture

Thanks, Mark! Appreciate all you've done and continue to do for the project.

markhalliwell’s picture

@hestenet, is there a way to get some browser stats for d.o?

I'd really (and I mean really) like to convert some of this stuff into straight ES6 (no babel transpiling).

I imagine that the majority of d.o users will be using a modern browser (Chrome, FF, Safari, Opera, Edge).

The reality is that Babel transpiling is actually now lower (percentage wise) than what modern browsers now support (mainly due to ES5 limits):

http://kangax.github.io/compat-table/es6/
https://caniuse.com/#search=es6

The only caveat will basically be "No IE support" and if d.o's stats can confirm this isn't an issue, I think this would be an OK route to pursue.

drumm’s picture

Here are the Google Analytics stats for www.drupal.org overall:
Google Analytics screenshot

Of the IE users, 97% of them are on IE11

markhalliwell’s picture

Awesome, thanks @drumm!

Of the IE users, 97% of them are on IE11

Unfortunately, IE11 still only has little to no native ES6 support. Which is why no IE "support" can be done without a transpiler.

I just want to avoid adding unnecessary and complex tooling for the infra team if we can avoid it. Would you agree?

That being said, this also means that over 95% of d.o users are already using modern browsers (this includes Edge and Opera).

I'd say this certainly qualifies as "acceptable" considering user enhancements are intended to be "newer" opt-in features anyway.

I've already figured out a way to detect IE, present a message/console error (if open), and dynamically load the scripts so it doesn't cause any syntax errors in the browser.

joachim’s picture

> I'd say this certainly qualifies as "acceptable" considering user enhancements are intended to be "newer" opt-in features anyway.

The other thing to consider is that the audience for dreditor's patch review features is a subset of the visitors of d.org: developers who are more likely to be using modern browsers.

markhalliwell’s picture

FTR, this is what I'll be using for a somewhat comprehensive ES6 "detection" support:

    /**
     * Checks whether the browser natively supports ES6.
     *
     * @see https://gist.github.com/DaBs/89ccc2ffd1d435efdacff05248514f38
     *
     * @return {Boolean}
     *   TRUE or FALSE
     */
    isEs6Supported: function isEs6Supported() {
      if (es6Support === null) {
        // Internet Explorer will never have native ES6 support.
        if (this.isIE()) {
          es6Support = false;
        }
        // Perform a fairly comprehensive test.
        else {
          try {
            new Function('class ಠ_ಠ extends Array {constructor(j = "a", ...c) {const q = (({u: e}) => {return { [`s${c}`]: Symbol(j) };})({});super(j, q, ...c);}}' +
              'new Promise((f) => {const a = function* (){return "\u{20BB7}".match(/./u)[0].length === 2 || true;};for (let vre of a()) {' +
              'const [uw, as, he, re] = [new Set(), new WeakSet(), new Map(), new WeakMap()];break;}f(new Proxy({}, {get: (han, h) => h in han ? han[h] ' +
              ': "42".repeat(0o10)}));}).then(bi => new ಠ_ಠ(bi.rd));');
            es6Support = true;
          }
          catch (e) {
            es6Support = false;
          }
        }
      }
      return es6Support;
    },

    /**
     * Determines if browser is Internet Explorer.
     *
     * @see https://stackoverflow.com/a/24861307
     *
     * @return {Boolean}
     *   TRUE or FALSE
     */
    isIE: function isIE() {
      return !!(navigator.appName === 'Microsoft Internet Explorer' || navigator.userAgent.match(/Trident/) || navigator.userAgent.match(/rv:11/) || ($.browser !== void 0 && $.browser.msie));
    },

edit: update code example

markhalliwell’s picture

The other thing to consider is that the audience for dreditor's patch review features is a subset of the visitors of d.org: developers who are more likely to be using modern browsers.

True, but I wanted the numbers from above to confirm my suspicions on this (just in case).

markhalliwell’s picture

Title: Implement User Enhancements on Drupal.org and port Dreditor features » [PP-2] Add Drupal+ to Drupal.org to utilize plus_enhancements for smaller Dreditor features
Status: Needs work » Postponed
Related issues: +#2947809: [Drupal+] Create a stable release
FileSize
55.02 KB

Ok, status update. I have been making significant progress but in other areas that will ultimately help support this issue.

I've been trying to think future forward here by working on consolidating a ton of code and solutions.

I briefly talked with @drumm and @hestenet in Slack and considering that Drupal.org already uses Composer Manager, it makes sense that the next step would be to also utilize something like Registry Autoload and Backport (formerly known as Service Container).

I've decided to abandon the User Enhancements (user_enhancements) namespace and moving it into Drupal+ as a submodule named Drupal+ Enhancements (plus_enhancements).

Drupal+ is to Drupal as jQuery Update is to jQuery.

It is a consolidation of code throughout the years in various projects I maintain where the normal out-of-the-box Drupal solutions is severely lacking. Yes, there are "modules for X", but specifically in regards to FE aspects (theme, drupal.js, AJAX, etc.)... there are usually only half backed solutions at best.

There are also a few other aspects Drupal+ will cover, namely around plugin improvements which have been successful in the Drupal Bootstrap base theme. Drupal+ will be a requirement for this base theme in the future due to #2938060: Abstract non-Bootstrap specific code into separate project which is a blocker of #2554199: Bootstrap 4. Needless to say, I have vested interest in getting all these projects to stable releases ;D

Thus, Drupal+ will allow Drupal.org to create dynamic and asynchronously loaded ES6 "Enhancement" plugins that don't affect the site's (d.o) aggregated JS bundles per each user's "setting preference". Naturally, Drupal+ will be dependent on Backport for its D7 "port" since Drupal 7 does not have annotated plugins ;)

There a lot of purposes behind everything I have said so far, but here are two major facts that benefit d.o directly:

  1. It will help make maintaining both D7 and D8 contrib modules much easier so we don't have to waste time on "backporting" functionality because Drupal.org is currently "stuck" on D7.
  2. Along the same lines, this gives Drupal.org a way to preemptively "upgrade" select pieces of the site to OO D8 code (or even use D8 modules), before actually doing a full upgrade. Thus, this will aid in alleviating some of the previous pain points that were encountered during the D6 -> D7 upgrade.

--

I am attaching a WIP POC patch which converts everything to ES6, supported by Drupal+, and Drupal 8 style plugins (YAML), supported by Backport. It technically doesn't "work" right now, but I just wanted to give y'all approximate code for what I am envisioning.

--

This all being said, this issue really should be postponed on #2947809: [Drupal+] Create a stable release, which is subsequently blocked by #2947810: [Backport] Create a stable release.

markhalliwell’s picture

Title: [PP-2] Add Drupal+ to Drupal.org to utilize plus_enhancements for smaller Dreditor features » [PP-1] Add user_enhancements to d.o for smaller Dreditor features
Assigned: markhalliwell » Unassigned

Considering there was absolutely no feedback on #66 in the past two years, I guess I can take that as a resounding "hell no".

So, after talking with @dww last night in Slack, I've decided to scale back the scope of this issue. Going back to #28 and the single https://www.drupal.org/project/user_enhancements approach (no ES6, D8, backport stuff).

Also, in an effort to avoid confusion with the previous "Dreditor" browser extensions and the new patch review code that was developed, the new "patch review" code has been split into a new separate project named "patchr" (ugh, naming things... will do for now I guess):

https://github.com/unicorn-fail/patchr
https://github.com/unicorn-fail/patchr-ui

https://github.com/unicorn-fail/dreditor still exists, but it has been archived and points to this issue in an effort to redirect people's effort to integrating features directly into d.o.

I'll see if maybe I can't spin up another dev instance to try and knock out some of the smaller dreditor features as #2788179: Implement "patchr" as a d.o user enhancement is a separate issue entirely.

drumm’s picture

Sorry for the lack of feedback earlier. The broad improvements in #66 were indeed a lot to consider all at once.

Having per-user switches for functionality isn’t really a direction I’d like to go in. For QA moving forward, this would give us a growing number of combinations to test, and potentially forget about. And for providing support and documentation, we would have an extra layer of figuring out what enhancements someone has enabled, or telling people they need to find an enable an enhancement.

I think the best way forward is to continue implementing features that are always on; or replacing functionality, for example with merge requests #2205815: Merge requests in Drupal.org issue queues?

markhalliwell’s picture

Status: Postponed » Closed (won't fix)

The only reason I implemented the toggleable user_enhancements is because we had all had a discussion about all this in IRC or slack or whereever. At that time, it was noted and majoritively agreed upon that not all developers liked the "features" of Dreditor and if we were to ever implement them, it would have to be in a fashion where they could disable the "enhancement".

So now it's going to be an "all or nothing" approach? Forgive me, but this kind of backpedling is frustrating as hell. Dreditor is dead; has been for years. People still use it though because not a lot of markup on d.o has change, however that is likely to change more and more as time goes on.

I'm really at a loss for what to do here. Its disheartening to continue to pour my time, energy and effort because people "demand" Dreditor remain alive, yet aren't actually willing to do a damn thing about it.

Guess this is a won't fix now.