If I click "Dashboard" in the shortcut menu, I get the link to add the Dashboard page to my current shortcut set. And if I click it, I will end up with two of them:

Multiple shortcuts

This should be fixed. :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Incidentally, IMO the way this should be fixed is for that add button to change to a delete button if I'm viewing a page that's already in my currently active shortcut set.

bleen’s picture

subscribe

bleen’s picture

Been working on webchick's suggestion and I'm about 75% there ... stay tuned

bleen’s picture

Assigned: Unassigned » bleen
Status: Active » Needs review
FileSize
1.31 KB
8.35 KB

Ok ... so I took webchick's suggestion and checked if the current page is already a shortcut in the current shortcut set. If it is, a "remove shortcut" link is displayed instead of an "add shortcut" link.

When reviewing this patch, please note that I had to change /modules/toolbar/toolbar.png in order to add a "minus" button to compliment the "plus" button. That PNG is also attached

Please review and let me know

Status: Needs review » Needs work

The last submitted patch failed testing.

bleen’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
8.35 KB

resubmitting the same patch because it passed here without my changing anything: http://drupal.org/node/612208

Bad testbot. BAD!

Please don't forget that I had to change /modules/toolbar/toolbar.png in order to add a "minus" button to compliment the "plus" button. I'm attaching that PNG again

Status: Needs review » Needs work

The last submitted patch failed testing.

bleen’s picture

Status: Needs work » Needs review

So this patch passed locally and passed here: http://drupal.org/node/612208

Chatter on IRC (boombatower & ilo---) suggests that testbot is very unhappy right now. So, I'm not sure exactly what to do now ... I guess, please review this patch. Would love for it to get into D7

bleen’s picture

yay testbot ... finally passed

webchick’s picture

Status: Needs review » Needs work

Functionality-wise, this looks great, and is exactly what I had in mind. Screenshots:

Unbookmarked page:
Unbookmarked page
Unbookmarked page (hover over add icon):
Unbookmarked page (hover over add)
Bookmarked page:
Bookmarked page
Bookmarked page (hover over remove icon):
Bookmarked page (hover over remove icon)

However, if I go to edit shortcuts, and create a new toolbar set and start playing with it, I start getting errors, like:

# Notice: Undefined property: stdClass::$links in shortcut_page_build() (line 535 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
# Warning: Invalid argument supplied for foreach() in shortcut_page_build() (line 535 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).

These errors no longer occur when I revert the patch, so this still needs some work.

Implementation-wise, hard-coding the visual styling of the shortcut bar in toolbar.png seems a bit odd. First, because shortcut is its own module, but secondly, I'm sure these icons look assy in something like Garland. However, this isn't really for this patch to solve, since you're just hinging off the existing implementation. Would be good to have a follow-up issue to discuss this though. It seems like shortcut module ought to only offer text links and for themes to style these however they'd like.

The shortcut_link_remove_inline() function makes me a bit nervous. Normally, we would stick confirm forms around any kind of deletion actions, so some "clever" dipstick on your website can't make an image in their comment pointing to admin/config/system/shortcut/1/remove-link-inline and start tricking an admin into deleting their links by looking at it (CSRF vulnerability). It's unclear to me whether all that various token-checking stuff solves this issue. Is this copy/pasted from the delete link on the, or..?

I need to spend some more time studying the changes to shortcut_page_build(), which I don't really have time to do now. It'd be great instead to pull in Jacob Singh or David Rothstein or one of the others who worked on the initial patch to get their thoughts. But as a general statement, watch your coding standards compliance... comments should end in a period and begin with a capital letter, there should be spaces around function calls, else goes on a separate line, etc.

webchick’s picture

Also, I ran into #613662: Shortcut item mis-alignment while testing this patch. I'm not sure if it's caused by this patch or not, so I made it a separate issue. Could you check?

bleen’s picture

However, if I go to edit shortcuts, and create a new toolbar set and start playing with it, I start getting errors, ...

I'll start with this and then take a look at addressing your thoughts about shortcut_link_remove_inline:

The shortcut_link_remove_inline() function makes me a bit nervous. Normally, we would stick confirm forms around any kind of deletion actions...

There is already a set of confirmations in place for when you use the edit shortcuts links

I agree with your comment about the images living in toolbar.png but I figured I'd go with the flow on that one for now.

re: coding standards... I'm still trying to break some old habits :) I'm getting there - slowly

bleen’s picture

Status: Needs work » Needs review
FileSize
9.3 KB

This patch fixes the errors that webchick was seeing when she added another shortcut set. This patch also takes care of the misalignment of the "+" links (#613662: Shortcut item mis-alignment). I'm still thinking about the best way to tackle the suggestion of utilizing confirmation form(s) when deleting a shortcut.

bleen’s picture

Waaaay easier than I thought to utilize the confirmation forms for deleting a shortcut. This patch should be real close... please to review, comment, etc..

David_Rothstein’s picture

Status: Needs review » Needs work

I haven't actually tried out the patch, just reviewed the code - but overall this looks pretty good! Here are some things I noticed, though:

tcut_link = $form_state['values']['shortcut_link'];
-  menu_link_delete($shortcut_link['mlid']);
+  _shortcut_link_delete($shortcut_link);
   $form_state['redirect'] = 'admin/config/system/shortcut/' . $shortcut_link['menu_name'];
-  drupal_set_message(t('The shortcut %title has been deleted.', array('%title' => $shortcut_link['link_title'])));
 }
 
 /**
+ * Helper function for deleting shortcuts.
+ */
+function _shortcut_link_delete($link){
+  menu_link_delete($link['mlid']);
+  drupal_set_message(t('The shortcut %title has been deleted.', array('%title' => $link['link_title'])));
+}

Breaking this out into a separate function doesn't seem that useful since it's only used once. I think maybe it is a relic from an earlier version of the patch that can now be removed?

-div.add-to-shortcuts a span.icon {
+div.add-remove-shortcuts {

I think something like "add-or-remove-shortcut" might be better (here and elsewhere)?

+div.remove-shortcut a span.icon{
+	background-position: -62px -60px;
+}

Code style (here and elsewhere): Space before the {, and an indentation of two spaces (rather than a tab) on the next line.

-  // Otherwise, use the default set.
-  if (!$shortcut_set) {
+  
+  if ($shortcut_set) {
+    $shortcut_set = shortcut_set_load($shortcut_set->set_name);
+  }
+  else { // Otherwise, use the default set.
     $shortcut_set = shortcut_default_set($account);
   }

This is a nice little bugfix! - I wonder if in addition to the fact that it's needed here, it might solve some other bugs too. (But note code style issue - the code comment should be on its own line.)

As part of fixing this, it looks to me like we can also modify the database query that runs right above it:

  $query = db_select('shortcut_set', 's');
  $query->fields('s');
  $query->join('shortcut_set_users', 'u', 's.set_name = u.set_name');
  $query->condition('u.uid', $account->uid);
  $shortcut_set = $query->execute()->fetchObject();

With your fix, it is now pulling more information than we need from the database - since most of that information gets duplicated again in shortcut_set_load(). Actually, the best way to fix this might possibly be to add an optional $account parameter to shortcut_set_load() itself and have that function join against the {shortcut_set_users} table in the case where the $account parameter is provided. This is not necessary for this issue and it could be a followup, but worth trying if you're interesting in tackling the database API a bit.

-  if (shortcut_set_edit_access()) {
+  if (shortcut_set_edit_access()) { 

Here and in a few other places you are adding extra space at the end of a line, which adds a code style violation even while you are not changing the code itself :) There should be no whitespace at the end of a line of code, and blank lines should have no whitespace at all. I'm guessing these came in by accident while you were working on the patch, but it's best to try to remove them because it also makes the patch bigger and a bit harder to review.

+    else { // $mlid is set, so remove the link.
+      $query['token'] = drupal_get_token('shortcut-remove-link');

Since we're redirecting to a confirmation form in this case, I don't think we need to add a token (Drupal's form API already provides tokens automatically). We only need it for the "add shortcut" link because in that case it goes to a URL that executes the action directly.

Also, in this part of the code, there seems to be a fair amount of duplication between the "add" and "remove" cases. Consider whether that could be consolidated... although the existing code that was already there is, I must admit, pretty ugly anyway, so maybe not worth doing too much as part of this patch.

RCS file: /cvs/drupal/drupal/modules/toolbar/toolbar.png,v
retrieving revision 1.4
diff -u -p -r1.4 toolbar.png
Binary files /tmp/cvsiucope and toolbar.png differ
Index: themes/seven/page.tpl.php

Yeah, as mentioned above, the fact that this is in toolbar.png is pretty messed up - that was an oversight that we missed in the rush to the original patch. We should perhaps make a separate issue for decoupling the images so that shortcut.module has its own normal images for the plus and minus icons - that'll be a prerequisite to fulfill some of the followup cleanup work at #511286: D7UX: Implement customizable shortcuts anyway.

Overall: Despite all the minor nitpicks above, I think this patch is really good. Reusing the confirmation form certainly makes sense since it's already there. In theory, I think this would be a great place in Drupal to not use a confirmation form for deletion but rather to just do the action automatically (which should be safe as long as there is a token in the URL) and then provide an "undo" link - it's a case where an undo link would actually not be that hard to implement. However, that's more than we need to do for this patch, and maybe it's not a great idea to introduce the "undo" pattern here when it's not elsewhere in Drupal (even though overall it is a better pattern). So I think the confirmation form is certainly fine for now.

David_Rothstein’s picture

Actually, the best way to fix this might possibly be to add an optional $account parameter to shortcut_set_load() itself and have that function join against the {shortcut_set_users} table in the case where the $account parameter is provided.

OK, what I wrote there makes absolutely zero sense - I have no idea what I thought I was trying to suggest. Ignore, please :)

We do, however, want to change this query:

  $query = db_select('shortcut_set', 's');
  $query->fields('s');
  $query->join('shortcut_set_users', 'u', 's.set_name = u.set_name');
  $query->condition('u.uid', $account->uid);
  $shortcut_set = $query->execute()->fetchObject();

so that it no longer pulls everything from the table but instead just loads the set name -- since that is the only thing that needs to get passed in to shortcut_set_load() as a result of this patch.

bleen’s picture

@David_Rothstein

Thanks for the feedback... here are a couple of notes before I get cracking on some revisions:

Breaking this out into a separate function doesn't seem that useful since it's only used once. I think maybe it is a relic from an earlier version of the patch that can now be removed?

Actually I broke this function out because it was being used in two places in my previous patch and I forgot to remove it

I think something like "add-or-remove-shortcut" might be better (here and elsewhere)?

sold!

Since we're redirecting to a confirmation form in this case, I don't think we need to add a token...

remnants from my previous patch when deleting happened without a confirm form

Code style (here and elsewhere)

I know, I know ... I can't seem to get this right. Are there any good tools out there that just check this for me and then smack me when I get it wrong? Thanks for keeping on top of it though - only way Ill ever learn it

David_Rothstein’s picture

Great, looking forward to the next version!

For code style stuff, you might try the Coder module - I know it checks some of that for you, although I'm not sure exactly how much.

bleen’s picture

Status: Needs work » Needs review
FileSize
6.21 KB

Lets give this patch a whirl ... once we get this patch settled I'll open a new ticket and breakout the shortcut images from toolbar.png

bleen’s picture

FileSize
590 bytes
624 bytes
8.11 KB

Ok ... it seemed a little silly to open a whole new issue just for the toolbar.png issue, so I have included a fix for that in this patch. Please note the atached shortcut.png and the toolbar.png files which belong in the root of their respective module folders. (this toolbar.png is different than the one I attached above).

The CSS changes have also been made to accommodate...

Functionally though, this patch is identical to "remove_shortcut-609152_4.patch"

bleen’s picture

bump ... would love to get this into d7 before the doors shut

David_Rothstein’s picture

No worries, the doors never shut on critical bugs. This one is going to get solved one way or the other :)

I just took a quick look at the patch. The new images look nice! (Although when I copy them into my installation I'm not seeing the shortcut +/- icons for some reason.)

Anyway, only minor issues with the code that I can see right now:

t($link_pretext.' shortcuts')

This kind of thing doesn't work well for translators - as I understand it, it makes it difficult to correctly translate the text. I think you want to fully define all the translatable string separately - for example, t('Add to shortcuts') and t('Remove from shortcuts') - even if that means a tiny bit of code duplication between the "add" and "remove" sections.

Also, there are still some spacing issues in the CSS file (each style declaration should have two spaces before it - looks like you have some tabs and such in the patch) and there are blank lines introduced by this patch that consist of spaces or tabs, whereas it really should be just a blank line with nothing on it.

Those are overall pretty minor. I'd want to look at this one more time more carefully, but it looks pretty close to me. Thanks!

bleen’s picture

OK... I feel really good about this one :)

bleen’s picture

bump :)

Status: Needs review » Needs work

The last submitted patch failed testing.

bleen’s picture

Status: Needs work » Needs review
FileSize
8.04 KB

gonna try and retest this. I dont understand why the patch changed to "fail" 5 days later

Status: Needs review » Needs work

The last submitted patch failed testing.

seutje’s picture

Status: Needs work » Needs review
FileSize
7.04 KB

Rerolled to current head and manually tested, behaves as expected aside from one weirdness: when I click "remove shortcut..." it brings me to a confirmation page, but at this confirmation page I seem to be able to add a shortcut (that would theoretically point to this confirmation page), but when doing so, nothing changes even though I get the following notice:
Added a shortcut for Are you sure you want to delete the shortcut <em>Dashboard</em>?.

so maybe we should define patterns for pages that should never be added as a shortcut

this patch assumes that shortcut.png and toolbar.png was replaced by the ones in #20

bleen’s picture

Thanks for rerolling

David_Rothstein’s picture

Looks pretty good to me. In the attached, I rerolled the patch to fix a few remaining whitespace issues and also to update the database query to only fetch the one column we need (as discussed above).

I'd say it's RTBC now except for one thing... I noticed that there is still code in themes/seven/style.css that uses the old CSS ID:

/* Shortcut theming */
div.add-to-shortcuts {
  float: left;
  margin-left: 6px;
  margin-top: 6px;
}

It seems that probably just needs to be updated to use add-or-remove-shortcuts instead, but then I wasn't sure because I also noticed there's some other minor theming differences here. Compare the two attached screenshots. The (minor) differences in the shortcut bar and in the placement of the + and - icons actually might look better with the patch as is (meaning maybe we just want to delete the above stuff from the Seven theme altogether), but it also seems that the CSS changes here caused the tabs ("List" and "Configure") to dip down a bit too low so that the words there are almost getting cut off? So overall, this needs one more pass through the CSS to make sure it is correctly doing what it wants to be doing :)

After that, I think this is RTBC. The issue discussed in #28 is probably a tricky one to solve (and also not introduced by this patch - the confirmation form was there before and had the same problem then).

seutje’s picture

FileSize
7.41 KB

ups, did I forget seven's style.css? :x

updated that and changed it to padding so it'll override shortcut.css's padding-top (btw, should we move seven's in there or stick with the minimalistic padding-top?)

and I added the padding values that got lost on div#toolbar div.toolbar-shortcuts ul li a

-  padding: 5px 10px 5px 5px;
+  padding: 5px 10px;

this makes before & after look the same

bleen’s picture

Ok ... I just double checked the last patch that seutje provided everything looks good. The old CSS that David_Rothstein mentioned in #30 isn't there and the shortcut '+' & '-' line up correctly.

I think we're good to go

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Agreed.

The patch in #31 combined with modules/shortcut/shortcut.png and modules/toolbar/toolbar.png in #20 are ready to go from my perspective. Nice work!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ahhhh. Now that is much better! :D

Great work, folks! Committed to HEAD.

bleen’s picture

woo hooo... my first real patch :)

webchick’s picture

Oh, yay! Congratulations, bleen!

Now, on a completely unrelated note, I must go get some jello and feathers... *whistles innocently*

Status: Fixed » Closed (fixed)

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