Re: [Maposmatic-dev] [PATCH 6/6] Implement a brand new wizard for map creation

[Maxime Petazzoni]

* Thomas Petazzoni <address@hidden> [2010-08-05 00:54:27]:

>  def new(request):
>      """The map creation page and form."""
> -
>      if request.method == 'POST':

No-op, leave as before.

>          form = forms.MapRenderingJobForm(request.POST)
>          if form.is_valid():
>              job = form.save(commit=False)
>              job.administrative_osmid = 
> form.cleaned_data.get('administrative_osmid')
> -
> -            existing = helpers.rendering_already_exists(job)
> -            if existing:
> -                request.session['redirected'] = True
> -                return HttpResponseRedirect(reverse('job-by-id',
> -                                                    args=[existing]))
> -

I think it would be better to remove the cache cleany in a separate
commit placed before this one in the stack.

> +            job.stylesheet = form.cleaned_data.get('stylesheet')
> +            job.layout = form.cleaned_data.get('layout')
> +            job.papersize = form.cleaned_data.get('papersize')

What about recreate()? And in recreate, you'll need to handle the cases
where stylesheet/layout/papersize isn't defined if the user recreates an
"old" map.

> diff --git a/www/media/map_rendering_form.css 
> b/www/media/map_rendering_form.css
> new file mode 100644
> index 0000000..1db0266
> --- /dev/null
> +++ b/www/media/map_rendering_form.css

You're missing the licence notice here.

> @@ -0,0 +1,84 @@
> +ul#steplist {
> +  margin: 0;
> +  padding: 0;
> +  padding-top: 10px;
> +  padding-bottom: 10px;
> +  list-style :none ;
> +  text-align : left ;
> +  background-color: #cccccc;
> +}

Consistency! Also, you can merge your padding rules into:
  padding: 10px 0;

> +ul#steplist li
> +{
> +  margin: 0;
> +  display: inline;
> +  padding-top: 10px;
> +  padding-bottom: 10px;
> +  padding-left: 2px;
> +  padding-right: 2px;
> +  background-color: #cccccc;
> +}

Same here, merge your padding rules. Also, the opening brackets should
be at the end of the line (everywhere), see style.css for the general
guidelines.

> diff --git a/www/media/map_rendering_form.js b/www/media/map_rendering_form.js
> new file mode 100644
> index 0000000..5aa5fb2
> --- /dev/null
> +++ b/www/media/map_rendering_form.js
> @@ -0,0 +1,459 @@
> +/* coding: utf-8
> + *
> + * maposmatic, the web front-end of the MapOSMatic city map generation system
> + * Copyright (C) 2009 David Decotigny
> + * Copyright (C) 2009 Frédéric Lehobey
> + * Copyright (C) 2009 David Mentré
> + * Copyright (C) 2009 Maxime Petazzoni
> + * Copyright (C) 2009 Thomas Petazzoni
> + * Copyright (C) 2009 Gaël Utard
> + * Copyright (C) 2009 Étienne Loks
> + * Copyright (C) 2010 Pierre Mauduit

New file in 2010, you should update the dates to 2010 (and put Pierre
after Frédéric for alphabetical order). We should actually discuss how
we write the copyright notices, maybe we could use "Copyright (C)
2009,2010 The MapOSMatic developers" instead of listing everyone?

Some comments in this file would be good, too.

> +var currentPanel = 1;
> +
> +function mapTitleChange()
> +{
> +    if ($("#id_maptitle").val().length != 0)
> +     allowNextStep();
> +    else
> +     disallowNextStep();
> +}

Don't mix tabs and spaces. Spaces only, 4 spaces per level of
indentation.

Also, the move of the auto-suggest feature to this new file should be
done before in a separate commit.

> diff --git a/www/templates/maposmatic/new.html 
> b/www/templates/maposmatic/new.html
> index a883598..433763c 100644
> --- a/www/templates/maposmatic/new.html
> +++ b/www/templates/maposmatic/new.html
> -<p>
> -{% blocktrans %}<em>MapOSMatic</em> covers the whole world but we need
> -contributors to translate and adapt the few parts of <em>MapOSMatic</em>
> -that are country specific.{% endblocktrans %}
> -</p>
> -
> -<p>
> -{% blocktrans %}To select the city to be rendered, two modes are
> -available:{% endblocktrans %}
> -</p>
> -
> -<ul>
> -  <li>{% blocktrans %}<em>Using an administrative boundary</em>. It allows
> -  to get a map with precise boundaries of the city when such limits are
> -  available in the database. Otherwise, you need to use a bounding
> -  box.{% endblocktrans %}</li>
> -  <li>{% blocktrans %}<em>Using a traditional bounding box</em>.{% 
> endblocktrans %}</li>
> -</ul>
> -
> -<p>
> -{% blocktrans %}Once the rendering is submitted, you will be brought
> -to a page giving the status of your rendering request. As soon as the
> -rendering is completed (that might take some time depending on the queue
> -length), this page will contain links to the generated
> -map.{% endblocktrans %}
> -</p>

Why remove all this? It still applies I believe.

> -<span id="noadminlimitinfo">{% blocktrans %}Wondering why you can't choose 
> some of these results?<br />Their administrative boundaries are missing from 
> the OSM database.<br />Look at the <a 
> href="http://wiki.maposmatic.org/doku.php?id=faq";>FAQ</a> for more details.{% 
> endblocktrans %}</span>
> -<span id="noresultsinfo">{% trans "No results." %}</span>
> -

You'll still need those for the autosuggest, won't you?

- Maxime
-- 
Maxime Petazzoni <http://www.bulix.org>
 ``One by one, the penguins took away my sanity.''
Linux kernel and software developer at MontaVista Software

signature.asc
Description: Digital signature