use templates instead of theme functions

Yeah, separating the theme function into a preprocess function and a template file is a good thing!

But the patch doesn't apply after the last commit I made. And I rather see that we move the actual markup to the template, instead of putting it in $vars['output'].

Looks nice! But I have some more thoughts on this...

I could definitely see situations where one would like to modify the markup. I've recently started a discussion on the Mapstraction mailing list about a bug in setInfoDiv(). In a scenario where that function is implemented here, you might want to add a div inside the mapstraction map div to show additional information in. So the best thing would be to have as much of the markup as possible in the template file (as your patch does now).

And regarding the provider API scripts -- I'd like to see some kind of rewrite here... As of now, the scripts are included just before the map div, in the middle of the page. This creates a sudden break while loading the page request. An ideal solution to this would be to print the provider API scripts at the bottom of the page, so the page can render without any sudden breaks.

A possible solution would be something like this:

Break out the markup into a template file and keep all the logics in a preprocess function (as you already did). Then, in the preprocess, load all necessary API scripts into a function with a static variable (mapstraction_set_provider() or something similar) instead of a template variable. This function should keep track of which providers that are set. Then, in hook_footer(), print all scripts that was statically cached in mapstraction_set_provider() at the bottom of the page. That way, we don't need to add the scripts in the template file and the page seems to render faster.

What do you think about this?

Implemented in the new 2.x branch.

I can't get this to work. I copied the mapstraction-map.tpl.php file to my themes folder but none of my changes come up. If I make changes directly in the module's directory then it's working as expected. And yes, I've cleared cache etc.

I can confirm the bug, but have no clue as to the cause? Anyone else? I'll try again tomorrow with a fresh set of eyes.

Alright, figured it out and it's now resolved. For anyone else, here's the deal. The array key within the theme definition won't accept a hyphen, it wants an underscore. E.g., In the following two snippets, the first version works correctly, but the second doesn't.

function mapstraction_theme($existing, $type, $theme, $path) {
  return array(
    'mapstraction_map' => array(
      'arguments' => array('api_script' => NULL, 'map_id' => NULL, 'width' => NULL, 'height' => NULL),
      'template' => 'mapstraction-map',
    ),
  );  
}

function mapstraction_theme($existing, $type, $theme, $path) {
  return array(
    'mapstraction-map' => array(
      'arguments' => array('api_script' => NULL, 'map_id' => NULL, 'width' => NULL, 'height' => NULL),
      'template' => 'mapstraction-map',
    ),
  );  
}

However, they both work when the function theme('mapstraction-map') is called from the same module.