widelands-dev team mailing list archive
-
widelands-dev team
-
Mailing list archive
-
Message #12020
Re: [Merge] lp:~widelands-dev/widelands/fh1-editorhelp into lp:widelands
Diff looks good, only a few minor suggestions.
Ingame it looked mostly good, too. Two small issues (okay, I am picky here):
1) In my opinion there is sometimes too much space before the list of trees. See comment below.
2) On 800x600 in the editor help, section "Tips", the last line starts with " the west" (notice the space). The same happens above with " + mouse click". Seems like the space normally gets some room at the end of the line, except that the previous line is too long in this two cases.
If you agree with the first one I think it can be fixed here, the second one is probably relatively big and deserves an own branch.
Diff comments:
> === modified file 'data/scripting/editor/editor_controls.lua'
> --- data/scripting/editor/editor_controls.lua 2017-08-16 05:33:50 +0000
> +++ data/scripting/editor/editor_controls.lua 2017-12-16 17:04:57 +0000
> @@ -10,50 +10,40 @@
> return {
> title = _"Controls",
> text =
> - rt(
> - h2(_"Keyboard Shortcuts") ..
> - p(
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey("F1"), _"Help") ..
> - -- TRANSLATORS: This is an access key combination.
> - dl(help_format_hotkey("H"), _"Toggle main menu") ..
> - -- TRANSLATORS: This is an access key combination. The hotkey is 't'
> - dl(help_format_hotkey("T"), _"Toggle tools menu") ..
> - toggle_minimap_hotkey ..
> - toggle_building_spaces_hotkey ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey("Ctrl + 1"), _"Toggle building spaces") ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey("Ctrl + 2"), _"Toggle immovables display") ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey("Ctrl + 3"), _"Toggle animals display") ..
> - -- TRANSLATORS: This is an access key combination. The hotkey is 'p'
> - dl(help_format_hotkey("Ctrl + 4"), _"Toggle resources display") ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey("P"), _"Toggle player menu") ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey("Ctrl + Z"), _"Undo") ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey("Ctrl + Y"), _"Redo") ..
> - -- TRANSLATORS: This is an access key combination. The hotkey is 'i'
> - dl(help_format_hotkey("I"), _"Activate information tool") ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey(pgettext("hotkey", "Ctrl + L")), _"Load map") ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey(pgettext("hotkey", "Ctrl + S")), _"Save map") ..
> + h2(_"Keyboard Shortcuts") ..
> + p(
> + -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> + dl(help_format_hotkey("F1"), _"Help") ..
> + -- TRANSLATORS: This is an access key combination.
> + dl(help_format_hotkey("H"), _"Toggle main menu") ..
> + -- TRANSLATORS: This is an access key combination. The hotkey is 't'
> + dl(help_format_hotkey("T"), _"Toggle tools menu") ..
> + toggle_minimap_hotkey ..
> + toggle_building_spaces_hotkey ..
> + -- TRANSLATORS: This is an access key combination. The hotkey is 'p'
> + dl(help_format_hotkey("P"), _"Toggle player menu") ..
> + -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> + dl(help_format_hotkey("Ctrl + Z"), _"Undo") ..
> + -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> + dl(help_format_hotkey("Ctrl + Y"), _"Redo") ..
> + -- TRANSLATORS: This is an access key combination. The hotkey is 'i'
> + dl(help_format_hotkey("I"), _"Activate information tool") ..
> + -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> + dl(help_format_hotkey(pgettext("hotkey", "Ctrl + L")), _"Load map") ..
> + -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> + dl(help_format_hotkey(pgettext("hotkey", "Ctrl + S")), _"Save map") ..
> toggle_fullscreen_hotkey
> - ) ..
> + ) ..
>
> - h2(_"Tools") ..
> - p(
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey(pgettext("hotkey", "0-9")), _"Change tool size") ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey(pgettext("hotkey", "Click")), _"Place new elements on the map, or increase map elements by the value selected by ‘Increase/Decrease value’") ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey(pgettext("hotkey", "Shift + Click")), _"Remove elements from the map, or decrease map elements by the value selected by ‘Increase/Decrease value’") ..
> - -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> - dl(help_format_hotkey(pgettext("hotkey", "Ctrl + Click")), _"Set map elements to the value selected by ‘Set Value’")
> - )
> + h2(_"Tools") ..
> + p(
> + -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> + dl(help_format_hotkey(pgettext("hotkey", "0-9")), _"Change tool size") ..
> + -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> + dl(help_format_hotkey(pgettext("hotkey", "Click")), _"Place new elements on the map, or increase map elements by the value selected by ‘Increase/Decrease value’") ..
> + -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> + dl(help_format_hotkey(pgettext("hotkey", "Shift + Click")), _"Remove elements from the map, or decrease map elements by the value selected by ‘Increase/Decrease value’") ..
> + -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> + dl(help_format_hotkey(pgettext("hotkey", "Ctrl + Click")), _"Set map elements to the value selected by ‘Set Value’")
> )
Haven't read this part in detail, seems to only be less whitespace and a removed rt().
> }
>
> === modified file 'data/scripting/editor/terrain_help.lua'
> --- data/scripting/editor/terrain_help.lua 2016-09-01 15:23:25 +0000
> +++ data/scripting/editor/terrain_help.lua 2017-12-16 17:04:57 +0000
> @@ -59,21 +59,22 @@
>
> local tree_string = ""
> for k,v in ipairs(tree_list) do
> - tree_string = tree_string .. picture_li(v.tree.representative_image,
> - v.tree.species .. ("<br>%2.1f%%"):bformat(100 * v.probability)) .. spacer()
> + tree_string = tree_string .. li_image(v.tree.representative_image,
> + v.tree.species .. ("<br>%2.1f%%"):bformat(100 * v.probability)) .. vspace(3)
> end
>
> -- TRANSLATORS: A header in the editor help
> - result = result .. spacer() .. rt(h2(_"Probability of trees growing")) .. spacer()
> + result = result .. vspace(3) .. h2(vspace(12) .. _"Probability of trees growing") .. vspace(3)
We have quite a lot of spacing here. Is it intentional?
In my opinion (note: I have no idea about ui design) it is a bit too much when there is no water resource on a terrain (e.g., mountains). The problem(?) is the combined space after the resource icon (ore/fish) and before the tree header.
>
> if (tree_string ~="") then
> result = result .. tree_string
> else
> - result = result .. rt(p(_"No trees will grow here."))
> + result = result .. p(_"No trees will grow here.")
> end
> +
> return {
> title = terrain.descname,
> - text = result
> + text = div("width=100%", result)
> }
> end
> }
>
> === modified file 'data/scripting/richtext.lua'
> --- data/scripting/richtext.lua 2017-07-03 10:16:59 +0000
> +++ data/scripting/richtext.lua 2017-12-16 17:04:57 +0000
> @@ -307,9 +307,9 @@
> -- :returns: a p tag containint the formatted text
typo "containint"
> function li(text_or_symbol, text)
> if text then
> - return p(text_or_symbol .. " " .. text .. vspace(6))
> + return div(p(text_or_symbol)) .. div(p(space(6))) .. div("width=*", p(text .. vspace(6)))
> else
> - return p("• " .. text_or_symbol .. vspace(6))
> + return div(p("•")) .. div(p(space(6))) .. div("width=*", p(text_or_symbol .. vspace(6)))
> end
> end
>
> @@ -336,12 +336,13 @@
> -- :arg text: the text to be placed next to the image
> --
> -- :returns: the text wrapped in a paragraph and placed next to the image, The outer tag is a div.
Typo: "The"
> -function li_image(imagepath, text_width_percent, text)
> - return p("<br>") .. div("width=100%", "") ..
> - div(p(img(imagepath))) ..
> +function li_image(imagepath, text)
> + return
> + div("width=100%",
> + div(p(vspace(6) .. img(imagepath) .. space(6))) ..
> div(p(space(6))) ..
> - div("width="..text_width_percent.."%", p(text)) ..
> - div("width=100%", "")
> + div("width=*", p(vspace(6) .. text .. vspace(12)))
> + )
> end
>
> -- RST
>
> === modified file 'data/tribes/scripting/help/introduction.lua'
> --- data/tribes/scripting/help/introduction.lua 2016-04-02 08:21:25 +0000
> +++ data/tribes/scripting/help/introduction.lua 2017-12-16 17:04:57 +0000
> @@ -1,13 +1,11 @@
> set_textdomain("tribes_encyclopedia")
>
> -include "scripting/formatting.lua"
> +include "scripting/richtext.lua"
> include "txts/help/common_helptexts.lua"
>
> return {
> title = _"About Widelands",
> text =
> - rt(
> help_introduction() ..
In editor_controls.lua you changed some whitespace after removing the rt().
(Not that I care about it.)
> help_online_help()
> - )
> }
>
> === modified file 'src/graphic/text/rt_render.cc'
> --- src/graphic/text/rt_render.cc 2017-12-10 03:03:53 +0000
> +++ src/graphic/text/rt_render.cc 2017-12-16 17:04:57 +0000
> @@ -678,7 +678,11 @@
> return 0;
> }
> std::shared_ptr<UI::RenderedText> render(TextureCache* /* texture_cache */) override {
> - NEVER_HERE();
> + // TODO(GunChleoc): When using div width=*, some newline nodes are not being consumed.
> + // Since it is working as expected otherwise and I can't find the problem, let's fix this some other time.
> + // Testing can be done with the editor terrains/trees help
> + // NEVER_HERE();
If there is a bug report about it, maybe mention its number here?
> + return std::shared_ptr<UI::RenderedText>(new UI::RenderedText());
> }
> bool is_non_mandatory_space() const override {
> return true;
--
https://code.launchpad.net/~widelands-dev/widelands/fh1-editorhelp/+merge/335283
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/fh1-editorhelp into lp:widelands.
References
