Show language name as icon tooltip

So... the patch looks perfectly fine and valid. Is there a reason it's set to "needs work" and not "needs review"?

I think the tooltip is a good improvement, and I don't see any flaws in the patch off-hand, so I'll try and test it later. :)

People, I have the tooltips without any additional patch applied. I am simply using the Language Link Title module that's been around since mid 2009 and it works just fine.

Now, perhaps you can take a look at its code and see if you've missed something or if that module does it better and 'borrow' the code. Once this feature is implemented natively in the Language Icons module, you can talk to the developer of the Language Link Title to place a note on his module's page stating that the module isn't any longer required.

PS: I have created an issue to the Language Link Title module's queue to let the developer know and in order to track the progress of this feature: #685204: Merge this module's features with the Language Icons module.

The code looks nice - thank you. :) There are a few code style issues, but they are minor, and could easily be fixed before commit.

I am thinking, though, that perhaps something like simply adding

    if (variable_get('languageicons_show_tooltip', TRUE)) {
      $attribs['title'] = $title;
    }

would suffice?

Well, I thought it was obvious... but I meant instead of this:

-    $attribs = array('class' => 'language-icon', 'alt' => $title);
+    if ( variable_get('languageicons_show_tooltip', TRUE) ) {
+      $attribs = array('class' => 'language-icon', 'alt' => $title, 'title' => $title);
+    } else {
+      $attribs = array('class' => 'language-icon', 'alt' => $title);
+    }

Excellent work trisales! Thank you. Just a thought here...

some users already use both modules on their drupal websites for some time now. If they never visit the modules' pages they won't know that this feature of Language Link Title is included in Language Icons. So they might continue using both. This might not cause any issues other than perhaps performance, but on the other hand it might.

What I suggest is that once your module's features are included in one future version of Language Icons, then you should release another final version of it that the only thing it will do is detect the version number of Language Icons used by the site and then warn the site admin. There could be a message thrown saying something like:

'You are using Language Icons version 6.x that includes all of Language Link Title module's features. Please disable and uninstall Language Link Title'.

What do you think about that?

Also I think you should co-maintain the Language Icons module or at least have your name mentioned somewhere in the module's credits. I believe this is only fair. What do you think freso?

I just committed the attached patch (with the change from comment #8).

However, is there a reason to even make this a setting, instead of simply showing it? By simply showing it, we remove an if-"loop", a few calls to the database, less clutter on the settings page as well as no additional strings. Which would mean that it could be back-ported to 6.x-1.x as well.

But there is no existing title to be wary of, that I can find:

function theme_languageicons_icon($language, $title = NULL) {
  if ($path = variable_get('languageicons_path', drupal_get_path('module', 'languageicons') .'/flags/*.png')) {
    $src = base_path() . str_replace('*', $language->language, $path);
    $title = $title ? $title : $language->native;
    $attribs = array('class' => 'language-icon', 'alt' => $title);
    if (variable_get('languageicons_show_tooltip', TRUE)) {
      $attribs['title'] = $title;
    }
    if ($size = variable_get('languageicons_size', '16x12')) {
      list($width, $height) = explode('x', $size);
      $attribs += array('width' => $width, 'height' => $height);
    }
    return "<img src='$src' ". drupal_attributes($attribs) .' />';
  }
}

The title attribute we set here, is only set for the img element, and doesn't affect the title of the a element. Or is there something to it, that I am not seeing?

@tirsales (and anyone else who would like to, of course): Would you care to give this a quick look over and see if there's anything I missed? :)

Hey Freso, I believe there is a pretty good reason to have this as an option. You see if there is only flag icons displaying, it is only logical to display the tooltip. I cannot imagine people wanting to disable the tooltips for any other reason except due to personal taste (they simply don't like it).

On the other hand, if people have language names enabled next to the flag icons, the tooltip might simply get in their mouse's way. So, these people might want to have the option to disable the tooltip without hacking the module's code. That's where a ckeckbox in the module's settings page comes handy.

What do you think? Is this if loop such an extra burden to today's CPUs and such an overhead to the servers' SQL db? I don't know, I'm just saying'.

No, this little thing in itself isn't much of a burden - but OTOH, all the small things add up in the end. I do think, however, that giving users fewer, but more sensible, options is the path forward (I'm also co-maintaining Pathauto, which has way, way too many options to be called any kind of user friendly as it is).

If a lot of people chime in and say that the title attribute broke their site, I'll reconsider, but I hardly think the tooltip (which is browser dependent as well, so really should be a browser configuration whether the user wants them shown or not - but that's another discussion for another time and place) is intruding enough to be a bother for >90% of users...

Not to mention that the title attribute is being set in a theme function, which is easily overridden in a custom theme or module.

Updated patch after the commits of another patch tonight.

Just a minor thing... the 'Show tooltips' checkbox kinda doesn't seem to belong in the 'Add language icons' settings group. Really trivial, but still...

Heh. Doesn't really matter right now, as the latest patch attempts to "fix" this by removing the option altogether. :p

I've committed the patch from #22. Thanks for discussion and inputs on this, tirsales, klonos, and Andrew! :)