Issue 1170 - [adblockplus.org Anwiki to CMS migration] Migrate content

includes/interface.tmpl

+{#
+ # This file is part of the Adblock Plus website,
+ # Copyright (C) 2006-2015 Eyeo GmbH
+ #
+ # Adblock Plus is free software: you can redistribute it and/or modify
+ # it under the terms of the GNU General Public License version 3 as
+ # published by the Free Software Foundation.
+ #
+ # Adblock Plus is distributed in the hope that it will be useful,
+ # but WITHOUT ANY WARRANTY; without even the implied warranty of
+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ # GNU General Public License for more details.
+ #
+ # You should have received a copy of the GNU General Public License
+ # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ #}
+
+{% macro description(key_name, open="<p>", close="</p>") %}

[Wladimir Palant, 2015/02/24 20:19:40]

Nit: Looking at how this macro is used, having open="", close="" as default
values seems to make more sense. <p> and </p> can be specified explicitly in the
one case separators are required.

[kzar, 2015/02/26 19:50:17]

Done.

+  {% for i in range(100) %}
+    {% if i == 0 %}
+      {% set current_key_name = key_name %}
+    {% else %}
+      {% set current_key_name = "%s%d" % (key_name, i) %}
+    {%- endif %}

[Wladimir Palant, 2015/02/24 20:19:40]

Nit: Why the space trimming here? We don't do it elsewhere.

[kzar, 2015/02/26 19:50:17]

Since I refactored this code the trimming no longer makes sense. I removed most
of them but must have missed this one, removed.

+    {% if current_key_name in localedata %}
+      {{open|safe}}{{ current_key_name|translate(None, links[key_name]) }}{{close|safe}}

[Wladimir Palant, 2015/02/24 20:19:40]

Why are links a separate variable rather than part of the interface definition?
Method/property declarations should simply have a description_links property.

[kzar, 2015/02/26 19:50:17]

Well I think because it's not that simple. Interfaces can have descriptions,
return descriptions and descriptions for each of their arguments. Then for each
of those descriptions you can have multiple paragraphs which you need to
differentiate between as well.

This is possible but I'd really rather not go there, especially because it will
complicated the conversion script quite a lot as well. (Worth noting with this
that there are relatively few links inside descriptions in practice. Adjusting
the data structure in the preference and two interface templates later would be
way easier than messing with the conversion script at this stage.)

[Wladimir Palant, 2015/02/26 20:35:03]

Fair enough. Having links in a separate data structure makes editing harder but
I see how this is complicated right now.

+    {% endif %}
+  {% endfor %}
+{% endmacro %}
+
+{% macro return_type(return_type) %}
+  {% if return_type|truncate(1, True, "") == "I" %}
+    {{ return_type|linkify }}
+      {{ return_type -}}

[Wladimir Palant, 2015/02/24 20:19:40]

Nit: Again, why the space trimming here? It should have no impact on the way
things are displayed.

[kzar, 2015/02/26 19:50:17]

This one is required, otherwise the link has a trailing space causing the space
in between the return type and interface to be part of the link.

+    </a>
+  {% else %}
+    {{ return_type }}
+  {% endif %}
+{% endmacro %}
+
+{% block body %}
+  {% set interface=interface|parse_interface %}

[Wladimir Palant, 2015/02/24 20:19:40]

I seriously dislike how this passes parameters via global variables. How about
using template inheritance instead. So an interface file would do:

{% extends "includes/interface.tmpl" %}

{{ interface([
  ...
]) }}

So instead of a block body here there would be a macro interface() that could
take parameters properly.

[kzar, 2015/02/26 19:50:17]

Done.

+
+  <h2>{{ "methods_and_properties"|translate }}</h2>

[Wladimir Palant, 2015/02/24 20:19:40]

Nit: Call this string "toc_header"? The string identifier should reflect what it
is used for, not what its current value is.

[kzar, 2015/02/26 19:50:17]

Done.

+  <ul>
+    {% for property in interface %}
+      {% if property.type == "method" %}
+        <li>
+          {{ return_type(property.return_type) }}
+          <a href="#method_{{ property.name }}" class="methodname">{{ property.name }}</a>
+          (
+          {% for argument in property.arguments %}
+            {{ argument.type }} {{argument.name }}
+            {%- if not loop.last %},{% endif %}
+          {% endfor %}
+          )
+        </li>
+      {% else %}
+      <li>
+        {{ property.modifier }} {{ property.type }}
+        <a href="#prop_{{ property.name }}" class="propname">{{ property.name }}</a>
+      </li>
+      {% endif %}
+    {% endfor %}
+  </ul>
+
+  {% for property in interface %}
+    {% if property.type == "method" %}
+      <hr>

[Wladimir Palant, 2015/02/24 20:19:40]

Nit: Move <hr> out of the if block? We need a separator regardless of whether
this is a method or property.

[kzar, 2015/02/26 19:50:17]

Done.

+      <p id="method_{{ property.name }}">
+        {{ return_type(property.return_type) }}
+        <span class="methodname">{{ property.name }}</span>
+        (
+        {% for argument in property.arguments %}
+          {{ argument.modifier }} {{ argument.type }} {{ argument.name }}
+          {%- if not loop.last %},{% endif %}
+        {% endfor %}
+        )
+      </p>
+      {{ description(property.name + "Description") }}
+      <dl>
+        {% if property.version %}
+          <dt>Version:</dt>
+          <dd>{{ property.version }} and higher</dd>

[Wladimir Palant, 2015/02/24 20:19:40]

The two strings above should be translated.

[kzar, 2015/02/26 19:50:17]

I agree but it's not currently and this issue is to migrate the existing
content. Shouldn't these translations be a separate issue? (I created a meta
issue for the website migration we can put things like this under
https://issues.adblockplus.org/ticket/2035 )

[Wladimir Palant, 2015/02/26 20:35:03]

Heh, fair enough.

[kzar, 2015/02/26 21:49:13]

Cool, I've filed an issue https://issues.adblockplus.org/ticket/2054

+        {% endif %}
+        {% if property.arguments|length %}
+          <dt>Arguments:</dt>

[Wladimir Palant, 2015/02/24 20:19:40]

This string should be translated.

[kzar, 2015/02/26 19:50:17]

See above.

+          {% for argument in property.arguments %}
+            <dd>
+              <span class="argumentname">{{ argument.name }}</span>:
+              {{ description(property.name + "Argument" + argument.name + "Description", "", "") }}

[Wladimir Palant, 2015/02/24 20:19:40]

"updateExternalSubscriptionArgumenttitleDescription" is a rather messy string
and unreadable string identifier. How about using
"updateExternalSubscription-titleDescription" instead?

[kzar, 2015/02/26 19:50:17]

Done.

+            </dd>
+          {% endfor %}
+        {% endif %}
+        {% if (property.name + "ReturnDescription") in localedata %}
+          <dt>Returns:</dt>

[Wladimir Palant, 2015/02/24 20:19:40]

This string should be translated.

[kzar, 2015/02/26 19:50:17]

See above.

+          <dd>{{ (property.name + "ReturnDescription")|translate(None, links[property.name + "ReturnDescription"]) }}</dd>

[Wladimir Palant, 2015/02/24 20:19:40]

Why not use the description() macro here? You did it for arguments which are
pretty similar.

Again, how about "updateExternalSubscription-returnDescription" as string name,
for readability? Note that there cannot be a parameter called "return" - it's a
reserved keyword.

[kzar, 2015/02/26 19:50:17]

Done.

+        {% endif %}
+      </dl>
+     {% else %}
+      <hr>
+      <p id="prop_{{ property.name }}">
+        {{ property.modifier }} {{ property.type }}
+        <span class="propname">{{ property.name }}</span>
+      </p>
+      {{ description(property.name + "Description") }}
+    {% endif %}
+  {% endfor %}
+{% endblock %}