Hide Implicit Orders.

[billw]

So I am implementing this item from the Todo list http://wiki.openttd.org/Todo_list. I have a version working:
http://imgur.com/a/slT2Q




The implicit orders can't be hidden in Timetable window only order window, it works for Trains and Buses, all rest of functionality remains the same.
A couple of questions:
Should I make skip order when implicits are hidden also skip the implicits? At the moment it won't and the display stays the same till you get to the next non-implicit.
What scope should the hide state be stored at? Currently it is on the window and resets every time you open the window, but I could either make it static so it keeps state across the session (or goes in the save file), or make it per train, and save it to the save file (dunno how to do that yet).
Any other ideas?

Attached a rough version, still has some commented out code etc, so you can remember what used to be there. This is against latest trunk.

[Kogut]

I think that button should not both change text and be pressed in, it is a bit confusing.

[WWTBAM]

I think that button should not both change text and be pressed in, it is a bit confusing.

Totally agree. Is confusing like that.

[Zuu]

Assuming you plan to implement hiding implicit orders in the timetable window, I would a minimum sync those two windows.

I don't really see why one would like to have implicit orders shown for just one train for a longer period of time but not the others. (yes you may want to use it to monitor one train, but you probably don't do that past save/load and it is probably not too bad to have a single switch enable/disable all trains)

Thus, perhaps store the setting in openttd.cfg as an interface setting? Or store it in the save game as a global setting. Global storage works best if there is only one switch located in advanced settings -> interface. If there has to be a button in each window, either the global advanced setting need to give the default value or you need to sync all buttons somehow to not confuse the users.


/my personal reflections

[billw]

Okay final version for now, also added a small feature because it bugs me so much:
When you switch between Orders and Timetable, the window position is maintained now (unless one or both windows are pinned).

Each vehicle has its own setting for whether implicit orders are hidden which doesn't go into save file, and timetable window does not have hide implicit. Might come back and do that later, but right now I am switching to work on my fix for the Spring 2013 pack.

[Lord Aro]

A few coding style errors in there (// comments vs /* comments, extra spaces dotted around, tabs for initial indenting only, etc)

[Zuu]

For displayable_order_list, it may be a good idea to use GUIList instead of std::vector<>. Windows that list elements in OpenTTD which use sort or filtering do this by using GUIList and provide sort/filter methods. Have a look on for example the sign list window (sign_gui.cpp).

Code: Select all

-		return (sel <= vehicle->GetNumOrders() && sel >= 0) ? sel : INVALID_VEH_ORDER_ID;
+		if(sel == displayable_orders_list.size())
+			return vehicle->GetNumOrders();
+		return sel >= 0 && sel < displayable_orders_list.size()?  displayable_orders_list[sel].id : INVALID_VEH_ORDER_ID;

Three coding style issues:
1. "if(" => "if ("
2. You must use { and } unless a single statement is placed at the end of the same line as the if statement. (the "{" symbol should be located on the same line as the terminating ")" of the if-statement)
3. Insert a space before "?".

(These errors occur at other places too. So review your patch for these problems)

Code: Select all

-	bool can_do_refit;     ///< Vehicle chain can be refitted in depot.
-	bool can_do_autorefit; ///< Vehicle chain can be auto-refitted.
+	bool can_do_refit;		///< Vehicle chain can be refitted in depot.
+	bool can_do_autorefit;	///< Vehicle chain can be auto-refitted.

Why do you indent the comments one additional space? Is it to align it with the comment two lines above? However, that comment is not added by your patch. You shouldn't make unrelated white space changes, so leave these rows as they are. (if you want to fix this white space thingy, that is a separate patch)

As pointed out, review your multi-line comments and comments that are located on lines containing no code to ensure that they confirm with the coding standard.

Code: Select all

/*
 * This is a comment that you can span
 * over multiple lines. This type of comment
 * must be used when a comment is located
 * at lines with no code.
 */

Dummy(); // This is a comment that you can place after a line with code.

Last, I wonder why do you display a sub index for implicit orders? Perhaps you should explain the motivation for this. The order window is already filled with information so adding more should be done with care.

[Eddi]

Last, I wonder why do you display a sub index for implicit orders? Perhaps you should explain the motivation for this. The order window is already filled with information so adding more should be done with care.

when toggling hiding/showing implicit orders, the numbering of the explicit orders should not change, so a sub-index for implicit orders might be sensible.

however, i think the explicit orders should not show the .0, and possibly the implicit orders should show only the sub-index and some indentation instead.

[billw]

Okay coding style fixed up, and improved the presentation a bit (still not exactly happy with it).

[billw]

It just occurred to me:


Looks better I think, but maybe a bit confusing?

[Eddi]

maybe make the pointer to the implicit order grey as well? then the one for the explicit order doesn't have to be so big.

[billw]

I'm using BigGUI grf incase you didn't realize, so I think the arrows are smaller when default size. Also I think that change would go beyond the purview of this patch into more general restyling of the Order window.

[Eddi]

no, by "big" i meant "double arrow", you can keep this a single black for the explicit order, and a single grey for the implicit order.

[billw]

Well in that case I refer you to my second point . Its a style change not a functionality addition, and therefore a different patch I think.