Support from Acquia helps fund testing for Drupal Acquia logo

Comments

salvis’s picture

Category: support » feature

The normal click is already used for expanding/collapsing a node, and I'm not so happy to force this functionality onto everyone, wasting precious screen space to display clutter that is useful only occasionally.

moshe has written before that he would like to have krumo replaced by our own implementation. What do you think, moshe?

IAC, new features have to go into 7.x-1.x-dev first. And please, increase your chances for anyone to look at your uploads by posting them expanded.

DongIT’s picture

Ok, I've changed the path only to display on a right click event on the Krumo node. So the precious screen space is preserved. Also changed the code a little and attached a Drupal 7 version.

salvis’s picture

Version: 6.x-1.9 » 7.x-1.x-dev

Right click is much better, thank you. Now post a D7 patch instead of a zip file, please.

DongIT’s picture

FileSize
2.71 KB

Attached a D7 patch.

salvis’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, devel_krumo_path.patch, failed testing.

The last submitted patch, devel_krumo_path.patch, failed testing.

salvis’s picture

Please change all tabs into two spaces and remove all trailing spaces.

+++ krumo_path.js	19 Jun 2011 11:24:01 -0000
@@ -0,0 +1,56 @@
+ * Requires jquery version >= 1.3

I can't remember what version we have in D7. Please check and remove the comment if ours is new enough.

+++ krumo_path.js	19 Jun 2011 11:24:01 -0000
@@ -0,0 +1,56 @@
+ * Author Wouter S. van Dongen http:/ /www.dongit.nl ¶

Please remove this. Your authorship ('DongIT') will be documented in the commit message, but other than that we don't put names and website ads into our code.

DongIT’s picture

FileSize
2.69 KB

Removed trailing spaces, changed tabs into spaces and removed authorship.

salvis’s picture

Status: Needs work » Needs review

Thanks!

Status: Needs review » Needs work

The last submitted patch, devel_krumo_path.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review
FileSize
2.4 KB

Please use git to create your patches, and all patches must now be -p1. Here's a reroll. Not sure why the status didn't explain that this was a -p0 patch and that's why it failed.

This is the same patch rerolled with git diff.

salvis’s picture

Status: Needs review » Needs work
+++ b/devel.module
@@ -441,6 +441,7 @@ function devel_set_message($msg, $type = NULL) {
 function has_krumo() {
   // see README.txt or just download from http://krumo.sourceforge.net/
   @include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'devel') .'/krumo/class.krumo.php';
+  drupal_add_js(drupal_get_path('module', 'devel') . '/krumo_path.js');
   return function_exists('krumo') && !drupal_is_cli();
 }

It doesn't make sense to add the .js file if this function returns FALSE, does it?

DongIT’s picture

No it doesn't :) Please verify the attached patch.

salvis’s picture

Status: Needs work » Needs review
DongIT’s picture

Sorry I just noticed I submitted the wrong version of my patch. This should be the correct one.

DongIT’s picture

My patching skills need some work, sorry. This should (finally) be the correct patch.

salvis’s picture

Status: Needs review » Needs work

No problem. The first few patches are tough to get right, but I'm sure you'll get the hang of it.

diff --git a/devel.module b/devel.module
old mode 100644
new mode 100755

100644 is the usual file mode. Why do you change this?

+++ b/devel.module
@@ -440,8 +440,11 @@ function devel_set_message($msg, $type = NULL) {
+  if (function_exists('krumo') && !drupal_is_cli()) {
+  	drupal_add_js(drupal_get_path('module', 'devel') . '/krumo_path.js');
+  	return TRUE;
+  }

The http://drupal.org/coding-standards call for 2 spaces per indent level and no tabs.

@rfay: Shouldn't the testbot have caught this?

diff --git a/krumo_path.js b/krumo_path.js
new file mode 100755

100644 — the files shouldn't be executable.

@rfay: Another task for the testbot?

+++ b/krumo_path.js
@@ -0,0 +1,50 @@
\ No newline at end of file

Maybe add a newline at the end for good looks.

DongIT’s picture

Not sure why the file modes were changed. Changed the file modes, tabs/spaces and added the newline.

rfay’s picture

@salvis, @DongIT, the reason the file mode gets changed is that some git clients have core.filemode turned on. And then you get a cross-platform situation where the two platforms disagree on what modes are "normal".

So if everybody would do

git config --global core.filemode false

that will solve the situation.

Since devel has no tests, the testbot didn't do anything. And it doesn't care about filemodes either, just about whether simpletest runs successfully. And unfortunately, the coder component built into the testbots has never been deployed. So coder checks don't happen.

moshe weitzman’s picture

Actually, I committed devel's first test last week. http://drupalcode.org/project/devel.git/blob/refs/heads/7.x-1.x:/devel.test

salvis’s picture

Status: Needs work » Needs review

Our first test, yay!

Thanks, Randy!

+++ b/devel.module
@@ -440,8 +440,12 @@ function devel_set_message($msg, $type = NULL) {
-  @include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'devel') .'/krumo/class.krumo.php';
-  return function_exists('krumo') && !drupal_is_cli();
+  @include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'devel') .'/krumo/class.krumo.php';  ¶

Oops, now you've introduced a trailing space, which makes this line different from what it was before. We're trained to watch out for these things (among many others of course)...

But never mind, I'll take care of that. Let's mark it NR and see what the testbot says now.

Powered by Dreditor.

salvis’s picture

EDIT: deleted duplicate post.

salvis’s picture

(weird — there seems to be some kind of caching problem...)

salvis’s picture

(after submitting my #24 I've again seen comments up to #21 only! after a refresh I'm seeing them through #24)

salvis’s picture

The right mouse button is not working for me because the browser uses it for showing the context menu of the page.

Here's an attempt to use double-click instead (and I've also done a bit of coding style cleanup). It works reasonably well for me with Firefox.

The functionality is a hidden feature. It would be nice to advertise it discretely, without adding undue clutter or overhead. I don't have any good idea though...

I'm not fluent in jQuery and I don't see core using

jQuery(document).ready(function() { ... });

anywhere. Is this the correct idiom in the Drupal world?

salvis’s picture

rfay’s picture

#26: Yes, I'd say that Drupal.behaviors is the correct way to add javascript behavior in D7. http://drupal.org/node/756722

It would be very unusual to use .read() any more.

DongIT’s picture

Added drupal.behaviors, please verify

salvis’s picture

Oops, I posted the wrong patch — the one with the right-click, which is eaten by my browser (FF) to show the page's context menu.

Here's an updated version with the double-click instead of the right-click. What do you think of this?

DongIT’s picture

I think the double click is ok. Better than the right click as the appearing right mouse menu can be annoying. However, when a krumo node is already opened, when double clicking on it to show the path, the node first closes and opens again with the path. It would be nicer if just the path would appear. But I don’t think this open-close thing is an issue.

salvis’s picture

On my FF 3.6 the right mouse click is not passed on to JS at all, i.e. the node doesn't toggle, only the context menu opens.

The expanding/collapsing of the node along with the path toggling occur as you describe. Does anyone have an idea how to avoid it?

And I still would like to see a way to advertise this feature discretely. Any ideas?

DongIT’s picture

Perhaps a combination like control + left mouse click?

salvis’s picture

No, control + left mouse click is too obscure IMO.

 

Added missing comment, adjusted indenting to match Drupal's coding style, fixed last line to be

})(jQuery);

rather than

}(jQuery));

In the Core code I see only the latter. It's weird that both seem to do the same thing...
(Did I say I wasn't fluent in jQuery? Can someone explain this?)

 

I'm not sure what the correct name and location for the krumo_path.js file is. Should it be called devel.krumo_path.js? Should it go to the krumo directory?

Status: Needs review » Needs work

The last submitted patch, krumo_path_plugin.1192616.34.patch, failed testing.

salvis’s picture

Status: Needs work » Needs review

#34: krumo_path_plugin.1192616.34.patch queued for re-testing.

DongIT’s picture

Looking at the other files, the correct file should be named devel_kumo_path.js. The JS file should not go in the Krumo dir, because all files in this dir belong to the orginal Krumo project. The modules root dir is ok.

By the way, right click could still be used even if you're using right click for your context menu: http://api.jquery.com/event.stopPropagation/

salvis’s picture

By the way, right click could still be used even if you're using right click for your context menu

It's not me using right-click for my context menu. Firefox/Win eats the right-click and displays it's context menu. The right-click doesn't get through to jQuery, AFAICS.

DongIT’s picture

I'm on Windows with Firefox, I don't have any problems with the right click version. But double click is fine.

moshe weitzman’s picture

Can we add a help icon (like the modules page has on some rows) to each level and show the path when someone clicks on that?

salvis’s picture

Hmm, adding something to every row is not so pretty, especially because there are different Krumo themes that it would have to work with.

But maybe we could add the help icon at the bottom left (to the left of 'Called from') and provide a title (mouse-over hint) there, something like: "Click to expand element, double-click to show path." Is that possible in JS? It would be nice if we didn't have to change Krumo...

Do you agree with DongIT in #37:

Looking at the other files, the correct file should be named devel_kumo_path.js. The JS file should not go in the Krumo dir, because all files in this dir belong to the orginal Krumo project. The modules root dir is ok.

DongIT’s picture

I’ve added title tags that shows one of these hints:
Click to expand. Double click to show path.
Click to expand. Double click to hide path.
Click to collapse. Double click to show path.
Click to collapse. Double click to hide path.
Double click to show path.
Double click to hide path.

I’ve added a screenshot with 3 examples (my mouse pointer is missing in the screenshot).

DongIT’s picture

Accidentally used tabs in the above patch, converted to spaces.

salvis’s picture

Please let's try to discuss possible solutions before putting time and effort into code.

I've tried to give some direction in #26...

The functionality is a hidden feature. It would be nice to advertise it discretely, without adding undue clutter or overhead.

... and I'm concerned that the latest patch is

  • clutter rather than discrete (You can't move your mouse anywhere without having a hint in your face.)
  • overhead (I'm not sure about this, but doesn't this inflate the DOM in the browser and risk blowing up or at least slowing down the browser for large data structures sooner rather than later?)
  • over-engineered (Devel is not designed for dummies and its features should be simple and to-the-point.)

Every user would like to see the "Click to expand element, double-click to show path." information exactly once in his lifetime (ideal goal, not to be implemented verbatim). After that it's only a distraction, and the less he sees of it, the happier he is. If he can't figure out that clicking again will collapse the element, then he really should not use Devel.

No one, at least no one in the target group for Devel, will need any of the five other strings, or even take the time to contemplate all of them, except possibly to complain that they can't be translated.

 

Moshe hasn't commented on your #37, so I think...

Looking at the other files, the correct file should be named devel_kumo_path.js. The JS file should not go in the Krumo dir, because all files in this dir belong to the orginal Krumo project. The modules root dir is ok.

... should be the way to go.

DongIT’s picture

FileSize
8.6 KB

The only other more discrete and more 'efficient' option I can think of is by adding a message to the bottom of krumo (screenshot).

salvis’s picture

FileSize
6.53 KB

How about this (with the mouse hovering over the question mark icon from the modules list page)?

Is this possible, maybe even without touching the files in the krumo directory?

DongIT’s picture

Good idea, check the patch.

salvis’s picture

Title: Krumo path plugin » Make the Krumo element paths available for copy/pasting
Status: Needs review » Fixed

Ah, very nice! Committed to D8/D7.

I've changed the file name according to #37, removed the trailing spaces, fixed the file mode, and added a hyphen according to this discussion.

Thanks for pulling this through!

dvessel’s picture

Status: Fixed » Needs review
FileSize
1.03 KB

Tiny patch fixes the hard linking of help.png. It was giving errors in my server log since my install is in a sub-directory. Also added a missing semi-colon.

salvis’s picture

Status: Needs review » Fixed

Committed to D8/D7. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.