#597 Impovement in the editor: toolbar

[Micah Bunting]

added terrain, port, and chit toolbar.
changed resize.
added resize for top and right side of map.
resize buttons focus_on_click=false.
clear hexes on resize.
removes ports on adjacent hexes when hex clears when required.
swapped col and row variable names in build_map_resize.
fixed infinite loop when loading a map >MAP_SIZE.
added pictures to describe hex, node, and edge arrangement.
added nav definitions and changed functions to use them
removed calc_adjacent(hex->map, hex->x, hex->y, adjacent); from connect_network.

this is my first patch, please tell me what you think

[Roland Clobus]

Hello Micah,

Thanks for your patch.

The end result is very appealing. It is much easier to create maps.

Regarding the code I have some remarks:
- The code didn't build initially, gdk_pixmap_unref is deprecated, use g_object_unref instead
- Could you reformat the code with 'make reindent'? You introduce another coding style
- In common/map.[ch] you introduce a global variable, and many #defines to operate on it. Is it really necessary to introduce a global variable? Can the #define operations be replaced by regular functions? Are all these changes required just to introduce hex_free and move_hex? I'm in favor of applying the minimal required change.
- Why does guimap_find_edge require a static variable? And why is its result const?
- The expose functions in editor.c should not be copied, but be moved to some file in common/gtk
- You changed the order of change_width and change_height in in editor.c, which complicates the patch file.
- Could you use C-style comments instead of C++?
- For clearer naming: could you rename RESIZE_[START|END]_[ADD|SUB] to RESIZE_[INSERT|REMOVE]_[TOP|BOTTOM|LEFT|RIGHT] ?

- Regarding functionality: why isn't a terrain type and a dice number active per default? I see no need to maintain the old context menus when the toolbar is present.

The amount of comments is quite high, but I really like the added functionality.

Regarding the inclusion in the Subversion repository, I would suggest that you take another look at the patch. I'll be able to look at new modifications or be able to apply the requested modifications no earlier than Saturday, Februari 12.

With kind regards,
Roland Clobus

[Micah Bunting]

editor resize and toolbar v2

[Micah Bunting]

I have addressed all but one of the comments and two bugs that were in it.

The #define operations are replaced with functions. In my opinion the use of the functions makes the code easier to read and understand then using the arrays used in build_network and connect_network.

The expose functions in editor.c and legend.c have been moved into theme.c and theme.h. This adds a possibility of a future patch that displays the theme before selecting it.

I could not figure out how to use 'make reindent' I saw some info on it, but not enough to fully understand it. Can someone send me more info on it so I can use it in the future?

[Roland Clobus]

I've updated the patch.
The resize functionality is already in svn, the toolbar is still in this patch.
My changes:
- I've use cairo where needed to match the current code
- I've placed the image of the active terrain under the chits, in order to have the correct colours in the toolbuttons

Remarks:
- I seem unable to set the port direction
- It's not immediate apparent how I could modify a tile, perhaps a context menu is still needed for a tile.
- The state with inselected terrain type is not clear to me.
- The code introduces the concept of 'active hex', but it isn't made visible.

I think some worked-out specs (on the pio-devel mailing list) is needed before this patch can be applied.

[Roland Clobus]

Updated to svn 1659. Only the toolbar part remains

[Micah Bunting]

I updated toolbar patch with the following changes:

- gint terrain to Terrain terrain.

- removed sensitive changes to chit_buttons when a new terrain is selected, since selecting a new chit when a non chit terrain is selected, deselects the terrain.

- selecting port deselects chit.

- selecting a chit deselects port.

- redraw chits when selecting a port or chit while a non supporting terrain is selected.

- added documentation to added functions in editor.c.

- changes global current_hex to local hex.

- removes select_port_resource_cb, select_port_direction_cb, select_terrain_cb required only for the context menus.

I like the background images for the ports and chits. My idea was to have the background transparent, but fell short with a white background which clashes with the grey toolbar, especially when selected.

Regarding the remarks:
- When placing a port, the port is facing the closest edge to the click location, if a port can legally be placed there. if not it chooses an allowable direction.

- Tiles are modified by clicking on a hex, depending on what is selected on the toolbar. If only the terrain is selected, only the terrain is changed. If only the chit is selected, only the chit is changed. If only the port, only the port is changed.

- I do not believe the context menu is required. The context menu allows changing terrain, chit, port, port direction, and a random option. The toolbar allows for all except for the random option. I would like to replace the current random option with the random hex groups, which will provide more options when randomizing maps, as well as making the maps easier to edit.

- I do not know what you mean by the inselected terrain type or the active hex, but I think you are referring to the global variable current hex. The variable was from the old context menu and changed to local variables.

[Micah Bunting]

updated verson of toolbar patch.

[Micah Bunting]

Just a quick question, What is plan with this patch? The editor toolbar is easier and faster to use then the context
menu. I also want to update the hex group patch to work with this after it is committed in the repository.

[Roland Clobus]

I could not find your reply on 2011-07-17 in my e-mail archive, so I missed it...
I'll review your updated patch soon.

[Roland Clobus]

Finally, I've reviewed this patch...

The review was done on svn revision 1669, as that was the time this patch was written.

I like the toolbar, it really can make the design process of a new board layout easier.
I'm uncertain how the toolbar should behave... As noted earlier, I think a discussion on pio-devel@lists.sourceforge.net will make it easier to discuss the feature.

A follow-up mail will be sent...
---> Read on the mailing list for more

[Micah Bunting]

I have made an update to the patch,
Only one hex element is selectable at one time.
Context menus are added back in. The left menu is only enabled if no toolbar button is selected (since left click is used for the selected item).

[Roland Clobus]

When building, I'm seeing the following (./autogen.sh;make -k clean all 2>err):

editor/gtk/editor.c: In function ‘expose_select_chit_cb’:
editor/gtk/editor.c:296:10: error: ‘GtkWidget’ has no member named ‘window’
editor/gtk/editor.c:299:28: error: ‘GtkWidget’ has no member named ‘window’
editor/gtk/editor.c:303:12: warning: unused variable ‘theme’ [-Wunused-variable]
editor/gtk/editor.c: In function ‘expose_select_port_cb’:
editor/gtk/editor.c:329:10: error: ‘GtkWidget’ has no member named ‘window’
editor/gtk/editor.c:332:28: error: ‘GtkWidget’ has no member named ‘window’
editor/gtk/editor.c:346:10: warning: variable ‘poly’ set but not used [-Wunused-but-set-variable]
editor/gtk/editor.c: In function ‘build_select_bars’:
editor/gtk/editor.c:468:8: warning: unused variable ‘num’ [-Wunused-variable]
editor/gtk/editor.c:463:13: warning: unused variable ‘icon’ [-Wunused-variable]
editor/gtk/editor.c:462:18: warning: unused variable ‘theme’ [-Wunused-variable]
make[2]: *** [editor/gtk/pioneers_editor-editor.o] Error 1
make[2]: Target `all-am' not remade because of errors.
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

---

• I would like to see 0 errors and 0 warnings with the deprecation checks running.
• The initialisation of selected_hex_element.type = NO_HEX_ELEMENT is missing, when the editor is started and I click on a hex, the terrain changes to hill.
• Spelling, I've seen at least: numver->number, betoween->between
• Wouldn't is be easier to read/debug if HexElement.chit did contain the actual dice roll instead of a number that needs a calculation?
• Minor request: Could the toolbuttons have a tooltip?
• Instead of unselecting an item on the toolbar, could you add a button that would have the same behaviour as the current unselected state? (Left-click context menu and possibility to add nosetup hexes). Then there could always be an active toolbutton. If you add 'No setup hex' and 'No setup node', that button could have a caption like 'Use left click context menu' or 'Manual terrain selection'.

[Micah Bunting]

I have address the above issues.

The calculations for chit has been removed. It is now stored as the number value of the chit.
There was three ways to correlate chit_buttons[], the selected chit number, and the value of the chit.

A: Have chit number directly relate to chit_buttons[] and require a calculation for the value of the chit. This is how the original patch was written.

B: Have chit number directly relate to value of the chit and require a calculation for the chit_buttons[]. I think these calculations would occur more then option A, as such I think option A is better.

C: Expand chit_buttons to have 13 elements. This would have elements 0,1, and 7 unused and only be wasted space.

With this new patch, I went with option C. The reason way is it is easier to read/debug and we could fill the other three chits with the combo chits found in fishermen of catan, though these chips were fixed on the lake hexes. The last one could be used for doubles. Tool tip strings for these have already been written since the tool tip array have the same gapes as the gaps in the chits. Adding these chits is way beyond the scope of this patch, and should be added later.

I have not added no setup node/hex buttons. These can be added in a different patch.

Some names have been renamed to clarify names. New names also imply that the selected toolbar button can affect nodes and edges, not just hexes.

I do not have a good picture for the unselected toolbar button. Currently it is only a white box.

[Roland Clobus]

I'm sorry, but I've got some more comments... The patch is working as intended, but I have some comments about the coding.

• Why do you use RGB(1,1,1) instead of RGB(0,0,0) or gdk_cairo_set_source_color(cr, &white)?
• I've used the following style for the tooltips:
  • no period at the end of the sentence
  • infinitive instead of 3rd person (Place a ... instead of Places a ...)
• Please do not add translatable text for features that do not exist yet. They will confuse the translator who will be looking for the feature. If you want to use 'TODO', create a Feature Request, please don't use the source code.
• I would suggest "Places a any 3:1 port" -> "Place a 3:1 port"
• I would suggest to replace the technical 'no setup' with: "Use left click context menu and click on nodes to toggle no setup." -> "Select the terrain type with the context menu on the left click or toggle a node to exclude it from being used during the setup phase"
• Why don't you use a GtkRadioToolButton? That has automatic deselection and will remove the need to keep the static global variables for the toolbarbuttons.
• Can you rename 'ToolbarButton' to 'ToolbarButtonData'? It is not the toolbar button itself, but its data/action.
• Are you sure you'll want to have the special chits in between the regular chits, whenever they'll be implemented, and not at the end?
• How about adding a ToolbarButtonData* to the buttons, that way you can have a single buttonclick handler.
• Why is the code in get_hex_polygon_from_radius a near duplicate of get_hex_polygon? Both functions are internals for guimap, so they could share code.
• Along that line, why did you extract the function 'draw_port' with the code for drawing the connecting lines, instead of just 'draw_port_indicator' which only draws the circle with the contents?
• Could you replace the magic numbers 24 and 12 by some constants like BUTTON_HEIGHT and BUTTON_HEIGHT/2?

[Micah Bunting]

I have fixed the next set of hits, with the following alteration.

get_hex_polygon_from_radius was removed because it was no longer required when the facing lines were moved back to display_hex.