Jump menu should default to showing the active page
gausarts - July 20, 2008 - 15:49
| Project: | Jump |
| Version: | 6.x-1.x-dev |
| Component: | Miscellaneous |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
I love this module as allows more flexible placements than jumpmenu. But I seem to have trouble to keep the selected menu item stays active once selected. After selecting a menu, the jump menu always goes to the upmost level menu item.
Is this meant to be so or did I miss anything? Thanks
#1
This is a feature request for which I would be very happy to commit a patch if one appears before I get around to coding it up. I think Jump Menu handles this case, so tracking down the code shouldn't be too difficult.
#2
I've got a hack working for me that is certainly not patch worthy, but may get the job done for you as well.
In jump.module around line 42 where it says "#default_value' => '0'", I changed it to "'#default_value' => 'node/' . arg(1),". arg(1) is the nid arg(0) is 'node'. I didn't copy the post where I found this out sorry. It's under someone asking how to get the current node id. This works for me because all my option values are 'node/nid'. I use this with a bonus view I built so I can have dynamic jump menus based on node listings.
Hope this helps.
-justin
#3
Moved into the 6.x queue. 5.x is in maintenance mode. Once this gets fixed, it's probably an easy back-port.
#4
Is this possible yet?
#5
Not yet, but it will make its way into an upcoming release.
#6
This patch adds the feature requested by gausarts.
#7
This should work regardless of whether or not we're using a menu or sending in the list of options directly to
jump_quickly(). It should work for taxonomy as well.I think it can be done generically in jump_quickly_form() by checking for the current path and setting the default_value right in there -- that way it will work for menus, taxonomy and also custom option lists that are passed directly into
jump_quickly(). Does that make sense?So make sure to test with:
1. A jump menu based on a top-level Menu
2. A jump menu based on a Vocabulary
3. A custom list of options (you can use the one in the README.txt)
#8
This patch provides a solution that is common across all jump menu possibilities. The LCD of options from the menu system and taxonomy paths is to compare the current path against each option key. The patch was created against the most recent commit (revision 1.1.2.1 date: 2009-09-11 11:22:48 -0700;)
#9
This looks a lot like what I was thinking would work. Could you please test to make sure it works for things like node/33 and page/my-first-page where page/my-first-page is a URL alias for node/33? I haven't tried this yet, but that's the one thing I can think of that we need to make sure works. It should work whether the person puts node/33 or page/my-first-page in their options list. Let me know if this doesn't make sense -- I basically want to make sure it plays nicely with path/pathauto.
#10
Ok, the following patch includes the #284985: Jump menu should default to showing the active page feature as well as simpletest integration. The following tests have been written and pass:
Create a menu.
1) Verify there is a jump menu equivalent in admin/build/block
Add a story node (with no path), add the node to the menu.
2) Verify you can jump to the added story node and that the menu title is selected in the jump menu.
Add a vovabulary. Add a term to the vocabulary. Add the term to the jump menu.
3) Verify you can jump to a taxonomy term and that the term is selected in the jump menu.
Add a story node with a custom path. Add the story node with the node/ path in the menu.
4) Verify you can jump to the story node and that it is selected in the jump menu.
Add a story node with a custom path. Add the story node with the custom path in the menu.
5) Verify you can jump to the story node and that it is selected in the jump menu.
Am I missing anything?
It should be noted that these tests where done against the DRUPAL-6--2-8 branch of simpletest _NOT_ DRUPAL-6--2-9 (aka DRUPAL-6--2).
#11
I really like the idea of having the current page selected.
However i like to have right that feature to be optional / configurable either via api or variable.
If you like users to see a text like "Select a menu item" in any case, sticky current selection will reduce usability.
Since i'm having trouble with cvs and diff to make a complete patch (new folder, ...) i'm adding a patch in addition to your.
What do you think? (Please merge if appropriate)
#12
@miro_dietiker - good point.
@evoltech - please roll #11 in with your patch so the community and I have only 1 patch to test.
Thanks, you two!
#13
BAM! You want patches?! You got some!1!111!
@miro_dietiker - I made this a configureable setting from admin/settings/jump, default is on.
@marcp - add a test case for it to, omg we are _so_ AGILE!
#14
@evoltech - I liked miro_dietiker's idea of sending the flag through the
jump_quickly()API call. On our sites we often embed ajump_quickly()call in a PHP-based block, and I can see the use case where we'd want some of those blocks to show the default page and some of them to show the regardless of what page we were on.I also like your idea of having a configurable setting, but I think it should be "per block" as opposed to being for the whole site. I can see a case where we'd want the block provided by the jump module for some top level menu to always show but where there may be another top level menu or a vocabulary where we'd want the jump menu to show the active page.
So ... how about this?
1. Move the "Show active page in menu" into a per-block setting
2. Put the parameter on
jump_quickly()3. Adjust the test cases to make sense based on 1 & 2
Would it also make sense to keep the "Show active page in menu" as a global setting which would be used as the default value for blocks before they've been configured? And as the default value for the new parameter to
jump_quickly()?#15
Instead of looping,
+++ jump.module 22 Sep 2009 00:05:48 -0000@@ -76,13 +109,35 @@ function jump_quickly($name = 'navigatio
+ foreach ($options as $path => $name) {
+ $ct = 0;
+ $default = $path;
+ foreach (explode('/', $path) as $index) {
+ if (strcmp($index, arg($ct))) {
+ $default = '';
+ break;
+ }
+ $ct++;
+ }
+ if (!empty($default)) {
+ break;
+ }
why not:
if (isset($options[$_GET['q']])) {$default = $_GET['q];
}
This review is powered by Dreditor.
#16
aaron -- q is the requested (potentially aliased) URL.
While arg() provides the internal unaliased path.
So if the jumper contains aliases with the q approach, then it will fail to mark them active.
But yes i also looked at the code and thought there should be a better way to achieve all that...
evoltech - Why not matching the string as full string instead of exploded chunks?
#17
$_GET['q'] will always return the unaliased path; that happens in http://api.drupal.org/api/function/drupal_init_path/6.
#18
you could even check for the off-chance that someone is calling jump_quickly() manually with aliased paths with something like the following:
if (isset($options[$_GET['q']])) {$default = $_GET['q];
}
else if (isset($options[drupal_get_path_alias($_GET['q'])])) {
$default = drupal_get_path_alias($_GET['q'])]);
}
that would cover both instances.
about my only complaint with the current set-up is that it's difficult to use this API when you have queries in the path; i had to set the link array to absolute paths to use this module with the pager, for instance. i initially reworked the API to work as well with $link arrays as expected by l(), url(), and theme('links'), but decided i needed to launch the instance without waiting on a patch to go through. if you think it's worth it, i can send up the patch your way, but it seems like a fairly limited use case.
#19
This integrates the suggestions from #14 and #15
#20
Indeed i took the wrong words and i was wrong, aaron.
I'm using aliased pathes for the jumper - since user exposition of internal pathes is pretty ugly. So we really need to be able to select aliased URLs as active.
I still see a need to loop through all options to check for either aliased q or unaliased q match. Don't u?
Kinda (unchecked) might be correct?
$q = $_GET['q'];$q_alias = drupal_get_path_alias($q);
foreach ($options as $path => $name) {
if($path==$q) {
$default = $q;
break;
}
elseif($path==$q_alias) {
$default = $q_alias;
break;
}
}
#21
mir0_dietiker The use case you describe is exercised in the test case named testJumpNodePathAuto(). It pases as written from aaron's recommendation #15.
#22
i didn't run the simple tests but #19 looks good to me.
#23
I'm looking at this now and will commit a modified version of #19. Thanks, folks.
@aaron - I think your complaint in #18 will still be valid even after this fix, so please feel free to open up another issue. In the 5.x version there was some code that allowed (I think) for something similar but then I decided that this would only be an issue for folks calling
jump_quickly()so they could be expected to send their paths thruurl()before sending them intojump_quickly(). Regardless, I'd like to see what you've come up with.#24
Committed a variation on this. The main thing that was added was the ability to pass $active into
jump_quickly(). I updated the README, but it's still crummy.Also, I haven't tested the new tests.
Can anyone out there confirm that the latest works and the tests pass?
Thanks for pushing this!
#25
Also still need
hook_uninstall()to delete our variables. I think it'll be easier to just get rid of all the jump_activepageinmenu* variables with a single SQL statement instead of trying to manage them individually as blocks are added/deleted, etc...#26
Immediately migrated my custom jump code to jump.module.
Using unaliased pathes works. However we think that using aliases is the only correct way as we're exposing pathes to the user / HMTL.
jump.module uses drupal_goto for the redirect in function jump_quickly_form_submit() @159. If you read api for drupal_goto you'll alias pathes are not permitted.
http://api.drupal.org/api/function/drupal_goto/6
$path A Drupal path or a full URL.If your drupal installation is inside a folder (e.g. $base_path='testinstall') the drupal_goto will break. If the alias is "product/myproduct" and the alias path finally is "/testinstall/product/myproduct" drupal_goto will finally lead to "/testinstall//testinstall/product/myproduct" which is wrong.
Using absolute URLs solved the problem but i don't think that's the best way to do it.
Unaliasing the $path in jump_quickly_form_submit() leads to a working solution but still exposes the internal path after the redirect.
As long as you allow working with aliases, drupal_goto might be the wrong tool. I've also thought about reporting wrong behaviour in drupal_goto.
Any inputs appreciated.
#27
Well thinking a second time about it, realizing that drupal_goto calls url() and aliases the path.
So passing unaliased urls to drupal_goto would be ok... (i still want to have aliased pathes in the form!)
So checking drupal_get_normal_path() in before drupal_goto might be an option but we need to check return codes carefully.
#28
marcp RE#23, I must have misread above where you wanted the optional $active argument in jump_quickly(), I put it in jump_quickly_form(). I reviewed your revised jump.module commit, and it looks good, all tests still pass. The test case I think should be in it's own tests directory though as it is in the patch. I will be sure to check for trailing whitespace with coder and one-line statements like you do in future patches. RE #25 I will submit a patch for this today.
miro_dietiker RE#26, and #27. I will write a test case for this edge case and submit a patch if it does not work today.
#29
@miro_dietiker - Are you able to provide a patch off what's in CVS now that fixes this? Do we have a simpletest that fails in this situation?
#30
@miro_dietiker - I set up an installation as you described and was unable to reproduce a failure with path auto menu items as you describe. I believe that the jump module, or more specifically the menu subsystem stores the menu aliased paths in the db unaliased. As a result the href in the jump menu will be unaliased, after jumping to it though you will see the aliased path.
In trying to create an automated test case for this situation I couldn't figure out how to do it though.
Included is the patch against journal.install from @marcp's request in #25.
#31
@evoltech - thanks for the hook_uninstall() patch - it's been committed.
@miro_dietiker - we'd like to be able to reproduce your issue. if you are sending in a $options array to jump_quickly() via some PHP code, please post it here.
#32
evoltech: right. internal pathes are stored unaliased. i'm using the api to create custom jumpboxes (submitting $options, containing aliased urls).
I even think it is wrong to contain unaliased pathes in the form. remember, those URLs are accessible for the user (are in HTML) and represent internals. If we have aliases - the only thing that can be seen of a user should be the alias. So i'd even vote for a url() call for each jump_menu_get_menu_options and jump_menu_get_taxo_options...
For testing you really need to install drupal in a sub directory.
Providing an $options array to fail is as simple as this:
Array(
[0] => "< Choose a product... >"
[/new/prod-a] => "Product A"
[/new/prod-b] => "Product B"
)
Whereas "/new" is the base_path of the drupal installation. And "prod-a" is the alias leading to full path "new/prod-a". Please note that this $options is self-created. However this seems fully okay from a users perspective.. But drupal_goto behaves wrong due to the missing support for aliases in $path.
I can try to patch, but i'm very short in time currently. Also i'd like to understand your opinions since unaliasing everything (to make it aliased again after by drupal_goto) might not be the best way.
#33
Hmm... drupal_get_normal_path and drupal_get_path_alias do only play well without URLs containing base_path... this makes kinda sense to me.
Since url() is the only api i can find to add the base_path, there's none to remove it correctly..
If i change the _submit function as follows:
function jump_quickly_form_submit($form, &$form_state) {if (!empty($form_state['values']['jump_goto'])) {
$to = $form_state['values']['jump_goto'];
$base_path = base_path();
if(substr($to, 0, strlen($base_path))==$base_path) {
$to = substr($to, strlen($base_path));
}
drupal_goto($to);
}
}
Is it really our job to uncut base_path from a url? is there no api for that?
The base_path is getting removed - if any. And drupal behaves right. (even though we're passing a alias to drupal_goto instead of an internal url as specified by it.) I still can't believe we're the only module having this issue with drupal_goto.
#34
@miro_dietikermiro_dietiker I see what you are saying more clearly.
I am still unable to reproduce your edge case manually. Here are the steps that I took.
1) Made sure PHP filter was enabled.
2) Created a new block with the php filter content type with the following code in the body:
<?php
$options = array(
// In this example both 'wsod' and 'auto/matic.sex/machine' are path aliases ala the path module.
'wsod'=> 'white screen of death',
'auto/matic.sex/machine' => 'bringin auto back',
);
echo drupal_get_form('jump_quickly_form', $options, 1);
?>
I noticed that the page redirects no problem when clicking on the links. The main difference here is the base path you are using above. The test installation that I used was installed in the a subdirectory of the webroot called 'drupal', but noticed how I do not prefix the option keys with "/drupal". This is by design.
You bring up another point, regarding security of site internals with regards to the drupal menu system and interaction with the path module, which is right on, and should be addressed. I believe the right place to address this is with those modules and a fix should not be integrated into the jump module.
#35
I'm setting this to fixed. I think miro_dietiker's issue can be solved with either:
1. Removing the leading / from the paths for the options array
2. Sending the url keys thru the url() function like this:
<?php$options = array(
'' => '< Choose a product... >',
url('new/prod-a') => 'Product A',
url('new/prod-b') => 'Product B',
);
?>
I think the latter will also address aaron's concern in #18. If either of you feels like this is still an issue, please open up a new issue and I'll be happy to look at it again.
#36
Automatically closed -- issue fixed for 2 weeks with no activity.