widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

I don't speak Django, but I went through it with my proofreader glasses on and have some suggestions.

Diff comments:

> 
> === added file 'templates/wlmaps/edit_comment.html'
> --- templates/wlmaps/edit_comment.html	1970-01-01 00:00:00 +0000
> +++ templates/wlmaps/edit_comment.html	2016-02-10 22:42:48 +0000
> @@ -0,0 +1,27 @@
> +{% extends "wlmaps/base.html" %}
> +
> +{% block content %} 
> +<h1>Edit comment: {{ map.name }}</h1>
> +<div class="blogEntry">
> +    <form enctype="multipart/form-data" action="{% url wlmaps_edit_comment map.slug %}" method="post">
> +        <div>
> +		{{ form.uploader_comment.label_tag }}:
> +			<span class="posRight">
> +				<a href="/wiki/WikiSyntax" title="Opens new Tab/Window" target="_blank">
> +					<img src="{{ MEDIA_URL }}img/menu_help.png" alt="Help on Syntax" class="middle">
> +					Help on Syntax
> +				</a>
> +			</span>
> +		{{ form.uploader_comment }}
> +		</div>

Indentation doesn't match <div>

> +		{% if form.uploader_comment.errors %}
> +			<span class="errormessage">{{ form.uploader_comment.errors }}</span><br />
> +		{% endif %}
> +		{% csrf_token %}
> +        <button type="submit">
> +            <img src="{{ MEDIA_URL }}forum/img/send.png" alt ="Submit" class="middle" />
> +            <span class="middle">Submit</span>
> +        </button>
> +	</form>
> +</div>
> +{% endblock %}
> \ No newline at end of file
> 
> === modified file 'templates/wlmaps/map_detail.html'
> --- templates/wlmaps/map_detail.html	2015-02-18 22:30:08 +0000
> +++ templates/wlmaps/map_detail.html	2016-02-10 22:42:48 +0000
> @@ -33,23 +34,34 @@
>  
>  {% block content %}
>  <h1>Map: {{ map.name }}</h1>
> -<div class="blogEntry">
> -	<a href="{% url wlmaps_index %}">Maps</a> &#187;
> -	{{ map.name }}
> -	<br /><br />
> -	<img class="posLeft map" src="{{ MEDIA_URL }}{{ map.minimap.url }}" alt="{{ map.name }}" />
> -
> -	<div style="display: inline-block;">
> +<div class="blogEntry" style="padding-bottom: 3em">
> +	<div>
> +		<a href="{% url wlmaps_index %}">Maps</a> &#187; {{ map.name }}
> +	</div>
> +		<img class="posLeft map" style="float: left" src="{{ MEDIA_URL }}{{ map.minimap.url }}" alt="{{ map.name }}" />
> +	<div>
>  		<h3>Description:</h3>
> -		<p>{{ map.descr }}</p>
> -	</div>
> -	<br />
> -	<div style="display: inline-block;">
> +		<p>{{ map.descr|wl_markdown:"escape" }}</p>
> +	</div>
> +	{% if map.hint %}
> +	<div>
> +		<h3>Hint:</h3>
> +		<p>{{ map.hint|wl_markdown:"escape" }}</p>
> +	</div>
> +	{% endif %}
> +
> +	<div style="clear: left;">
>  		<h3>Comment by uploader:</h3>
> -		<p>{{ map.uploader_comment }}</p>
> -	</div>
> -	<br />
> -	<div style="display: inline-block;">
> +		<div>{{ map.uploader_comment|wl_markdown:"remove" }}</div>

Unindent this.

> +		{% if user == map.uploader %}
> +			<a class="button posLeft" href="{% url wlmaps_edit_comment map.slug %}">
> +				<img alt="Edit" title="Edit your comment" class="middle" src="{{ MEDIA_URL }}forum/img/edit.png">
> +				<span class="middle">Edit</span>
> +			</a>
> +		{% endif %}
> +	</div><br />
> +
> +	<div style="display: block" >
>  		<h3>Basic Information:</h3>
>  		<table>
>  			<tr>
> 
> === modified file 'wlmaps/forms.py'
> --- wlmaps/forms.py	2014-07-21 15:29:30 +0000
> +++ wlmaps/forms.py	2016-02-10 22:42:48 +0000
> @@ -26,44 +27,61 @@
>              # no clean file => abort
>              return cleaned_data
>  
> -        name = MEDIA_ROOT + "wlmaps/maps/" + file.name
> +        name = MEDIA_ROOT + 'wlmaps/maps/' + file.name
>          default_storage.save(name, ContentFile(file.read()))
>          try:
>              # call map info tool to generate minimap and json info file
> -            check_call(["wl_map_info", name])
> -            # TODO(shevonar): delete file because it will be saved again when the model is saved. File should not be saved twice
> +            check_call(['wl_map_info', name])
> +            # TODO(shevonar): delete file because it will be saved again when
> +            # the model is saved. File should not be saved twice
>              default_storage.delete(name)
>          except CalledProcessError:
> -            self._errors["file"] = self.error_class(["The map file could not be processed."])
> -            del cleaned_data["file"]
> +            self._errors['file'] = self.error_class(
> +                ['The map file could not be processed.'])
> +            del cleaned_data['file']
>              return cleaned_data
>  
> -        mapinfo = json.load(open(name + ".json"))
> +        mapinfo = json.load(open(name + '.json'))
>  
> -        if Map.objects.filter(name = mapinfo["name"]):
> -            self._errors["file"] = self.error_class(["A map with the same name already exists."])
> -            del cleaned_data["file"]
> +        if Map.objects.filter(name=mapinfo['name']):
> +            self._errors['file'] = self.error_class(
> +                ['A map with the same name already exists.'])
> +            del cleaned_data['file']
>              return cleaned_data
>  
>          # Add information to the map
> -        self.instance.name = mapinfo["name"]
> -        self.instance.author = mapinfo["author"]
> -        self.instance.w = mapinfo["width"]
> -        self.instance.h = mapinfo["height"]
> -        self.instance.nr_players = mapinfo["nr_players"]
> -        self.instance.descr = mapinfo["description"]
> -        self.instance.world_name = mapinfo["world_name"]
> -        # mapinfo["minimap"] is an absolute path and cannot be used.
> -        self.instance.minimap = "/wlmaps/maps/" + file.name + ".png"
> -
> -        # the json file is no longer needed
> -        default_storage.delete(name + ".json")
> -
> +        try:
> +            self.instance.name = mapinfo['name']
> +            self.instance.author = mapinfo['author']
> +            self.instance.w = mapinfo['width']
> +            self.instance.h = mapinfo['height']
> +            self.instance.nr_players = mapinfo['nr_players']
> +            self.instance.descr = mapinfo['description']
> +            self.instance.hint = mapinfo['hint']
> +    
> +            self.instance.world_name = mapinfo['world_name']
> +            # mapinfo["minimap"] is an absolute path and cannot be used.
> +            self.instance.minimap = '/wlmaps/maps/' + file.name + '.png'
> +    
> +            # the json file is no longer needed
> +            default_storage.delete(name + '.json')
> +        except KeyError:
> +            self._errors['file'] = self.error_class(
> +                ['A KeyError is occured. Please ask in the forum for help.'])

Is it possible to add info about which key is causing the problem to the error message?

> +            del cleaned_data['file']
> +            return cleaned_data
> +        
>          return cleaned_data
>  
> -
>      def save(self, *args, **kwargs):
>          map = super(UploadMapForm, self).save(*args, **kwargs)
>          if kwargs['commit']:
>              map.save()
>          return map
> +
> +
> +class EditCommentForm(ModelForm):
> +
> +    class Meta:
> +        model = Map
> +        fields = ['uploader_comment', ]
> 
> === modified file 'wlmaps/test_urls.py'
> --- wlmaps/test_urls.py	2009-04-04 14:32:27 +0000
> +++ wlmaps/test_urls.py	2016-02-10 22:42:48 +0000
> @@ -3,6 +3,5 @@
>  from django.conf.urls.defaults import *
>  
>  urlpatterns = patterns('',
> -    url(r'^wlmaps/', include("wlmaps.urls")),
> -)
> -
> +                       url(r'^wlmaps/', include('wlmaps.urls')),

Indent looks weird.

> +                       )
> 
> === modified file 'wlmaps/urls.py'
> --- wlmaps/urls.py	2009-04-11 21:41:28 +0000
> +++ wlmaps/urls.py	2016-02-10 22:42:48 +0000
> @@ -5,12 +5,16 @@
>  from views import *
>  
>  urlpatterns = patterns('',
> -    url(r'^$', index, name="wlmaps_index" ),
> -    url(r'^upload/$', upload, name = "wlmaps_upload" ),
> -    
> -    url(r'^(?P<map_slug>[-\w]+)/$', view, name = "wlmaps_view" ),
> -    url(r'^(?P<map_slug>[-\w]+)/download/$', download, name = "wlmaps_download" ),
> -    
> -    url(r'^(?P<map_slug>[-\w]+)/rate/$', rate, name = "wlmaps_rate" ),
> -)
> -
> +                       url(r'^$', index, name='wlmaps_index'),

Indent looks weird.

> +                       url(r'^upload/$', upload, name='wlmaps_upload'),
> +
> +                       url(r'^(?P<map_slug>[-\w]+)/$',

Can ^(?P<map_slug>[-\w]+) be refactored into a variable?

> +                           view, name='wlmaps_view'),
> +                       url(r'^(?P<map_slug>[-\w]+)/edit_comment/$',
> +                           edit_comment, name='wlmaps_edit_comment'),
> +                       url(r'^(?P<map_slug>[-\w]+)/download/$',
> +                           download, name='wlmaps_download'),
> +
> +                       url(r'^(?P<map_slug>[-\w]+)/rate/$',
> +                           rate, name='wlmaps_rate'),
> +                       )


-- 
https://code.launchpad.net/~widelands-dev/widelands-website/add_hints/+merge/285664
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands-website/add_hints into lp:widelands-website.