Zuu wrote:

Some comments on your code. There are some coding style errors which I have commented on. Please also see this documentation of the coding style: http://wiki.openttd.org/Coding_style

In overall I think it is a good work for not being experienced with C++. Most comments are with respect to the coding standard.

I have not yet applied your patch and actually tested it.

+DropDownList *GetIndustryTypeDropDownList(bool include_show_all=0){
=>
+DropDownList *GetIndustryTypeDropDownList(bool include_show_all = false)
+{
Comment: We only use 'true' or 'false' for boolean variables. Opening curly bracket of methods go on a new line. Also, please add doxygen comments to this method that document what it does and its parameters and return value.

+ if (this->selected_filter_industry_type_index == -1 || i->type == this->selected_filter_industry_type_index) {
Comment: Please only use tabs for indention of code.

+ switch (this->selected_filter_transported_index) {
Comment: Instead of commenting each case (1, 2, 3), you could define Enum constants for index 1, 2 and 3. This is a standard way to deal with lists of items in C++. The variable that holds an item from a such list then get the type of the enum instead of just a plain integer type. This adds to readability and structure of the code.

Line 99: -
Comment: Don' remove this white space line. (tip: review your own patches in a text editor before upload and you can detect unintended changes)

+ if (widget == WID_ID_DROPDOWN_FILTER_TRANSPORTED) SetDParam(0, IndustryDirectoryWindow::filter_names[this->selected_filter_transported_index]);
Comment: There is already another if statement that checks for another widget. So instead of adding another if-statement, you should turn both it to a switch-case statement with one case for each widget. Se for example DrawWidget of the same window.

+ case WID_ID_DROPDOWN_FILTER_INDUSTRY_TYPE: {
Comment: The code for this case doesn't define any local variables. thus the '{' and '}' is not needed here. A basic switch-case statement in C++ doesn't have those curly brackets. They are only added when a local scope for variables is needed for a case block.

+ switch (widget) {
+ case WID_ID_DROPDOWN_CRITERIA:
Comment: If you add control structures that change the correct indentation of existing code, the patch have to change those lines so that they get proper indentation.

+ // Needs to refresh the widget somehow
Comment: Use this->SetWidgetDirty(WID_ID_...)

+/* Names of the filter functions */
+const StringID IndustryDirectoryWindow::filter_names[] = {
+ STR_FILTER_TRANSPORTED_BOTH,
+ STR_FILTER_TRANSPORTED_ONLY_NOT,
+ STR_FILTER_TRANSPORTED_ONLY,
+ INVALID_STRING_ID
+};
Comment: In C++ you can have a ',' also after the last item in a list. This simplifies subsequent patches/changes to the code if all items in a list/enum have a comma after them.

+ WID_ID_DROPDOWN_FILTER_TRANSPORTED,
+ WID_ID_DROPDOWN_FILTER_INDUSTRY_TYPE,
Comment: Please add doxygen comments to these two enum constants. (eg. look at the lines just above/below)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11927

ComLock wrote:

I agree with your comments, I did this patch before reading your comments on #5453
I've just used the emacs editor for now, without setting up whitespace laws. Guess it's time to start using Eclipse.
I'll try to be less perl/ruby hacker and more obey the laws of the project to get you code approved :)

Hmm the weird thing is BuildSortIndustriesList does this->SetWidgetDirty(WID_ID_INDUSTRY_LIST)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11930

Zuu wrote:

this->SetWidgetDirty(WID_ID_INDUSTRY_LIST) will make the widget dirty and trigger a re-paint. However, if you changed the filter, the list may need a rebuild also, otherwise the same list will just get repainted. Have a look at other filter functions and see how it updates the list upon filter changes.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11932

ComLock wrote:

Hopefully there isn't to much wrong now :)

Attachments

• filter_industry_list_v2.diff (10.74 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11933

Zuu wrote:

+ WID_ID_DROPDOWN_FILTER_TRANSPORTED, ///< Dropdown for filtering on industry is transported or not
+ WID_ID_DROPDOWN_FILTER_INDUSTRY_TYPE, ///< Dropdown for filtering on industry type
Comment: In general new comments in OpenTTD should be full sentences. In this case the neighbouring lines have terminating periods, so use that also for your new comments.

+/** Alternatives for filtering by industry transported */
+enum { // Should this enum have a name? example: TransportedFilterSelections
+ TRANSPORTED_SHOW_BOTH, ///< Show both transported and non-transported industries
+ TRANSPORTED_SHOW_ONLY_NOT, ///< Show only non-transported industries
+ TRANSPORTED_SHOW_ONLY, ///< Show only transported industries
+};
+
Comment: Terminate sentences with period ('.')

+DropDownList *GetIndustryTypeDropDownList(bool include_show_all = false){
Comment: The curly bracket should move to text line. In the doxygen comment, also include a @param line for each parameter and a @return to document the return value. Look at existing methods to see how this is done.

+ static const StringID transported_filter_names[]; ///< List of filter by transported alternatives
+ byte selected_filter_transported_index; ///< The currently selected filter by transported industry
+ int selected_filter_industry_type_index; ///< The currently selected filter by industry type
+ Dimension ind_textsize; ///< Size to hold any industry type text, as well as STR_INDUSTRY_CARGOES_SELECT_INDUSTRY.
Comment: I think you can spot the missing periods here.

+ bool transported = 0;
Comment: I think you see what needs to be changed here.

+ } // for cargoes
Comment: For short loops there is no need to comment the terminating curly bracket. When you got code with 8 indentation levels that span multiple screens, then such comments may help to not get lost. But for short simple loops it is not necessary.

+ virtual void SetFilterTransportedIndex(byte index) {
+ this->selected_filter_transported_index = index;
+ }
+
+ virtual void SetFilterIndustryTypeIndex(int index) {
+ this->selected_filter_industry_type_index = index;
+ }
+
Comment: Please add doxygen comments to these methods.

+ } // switch
Comment: (Line 254 of patch) This comment could be removed. Once you get used to the switch-case syntax and indentation you don't need to see the top part of the switch statement to know that this is a switch block.

Regarding using Emacs or Eclipse, that is up to you and what you find easiest to use. I'm sure Emacs can be set up to work well with C++ programming and the OpenTTD coding style. Things like handling different type of indentation rules should be at the core of any text editor for programmers.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11939

ComLock wrote:

I'm feeling the pain of C++ programming :)

Anyways would it be a good idea to split industry_gui.cpp into 1 file per window...
I'm getting search and scroll crazy.
I'm not working on all the windows at the same time, just one, so there's a lot of stuff I don't want to eye scan trough all the time.

BTW: I've started programming filter on industry type, as is done in the smallmap window with legend, instead of dropdown.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11940

Alberth wrote:

Code style adjustments are never easy :)

A single file per window is not a very good idea, as we have about 120 windows :)

However, you are free to make any form of changes to the source while you develop. For example, you can add folding markers, or recognizable comment lines, or whatever you prefer in the file, just remove them just before the final check.

I prefer not to make such changes, and often make a note of the interesting line numbers, and use that as a guide. If your editor supports multiple windows of the same file, that's useful too to show relevant parts (and not much else).

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11941

Zuu wrote:

In some editors you can add bookmarks for files/lines so that you can jump back to the bookmark later. Another useful way to jump to where you want to go, is to make a search for a string that you know is found in this place and is somewhat unique in the source code.

I think this patch is on the borderline for if it should be split into two separate patches or not. One for each filter. However, I've not asked you to do so because the length of the patch so far is not that long and it is possible to commit "filter options for industry window". However if you plan to extend the patch further, it may be a good idea to split it into two parts.

To help with developing a series of sequential patches, I and several other people who code on OpenTTD use the queues add-in in Mercurial (hg). Others use git for the same purpose. With hq queues you basically tell hg which patch you are currently working with. The changes you make are then recorded into the correct patch. When trunk is updated you update the base version and can then apply your patches again one by one and fix any conflicts.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11942

ComLock wrote:

I would ofcourse like to use github... branching ... pull request
https://github.com/search?q=openttd&type=Repositories&ref=simplesearch

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11943

ComLock wrote:

How it looks now

Attachments

• filter_industry_list2.png (543.18 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11944

frosch wrote:

Some random chatter about what also might look nice: starting from 2013-02-01, 23:26 CET
http://irclogs.qmsk.net/channels/openttd/date/2013-02-01?page=4

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment11946

ntx wrote:

I also implemented my take on the industry list filter (only by industry type) before I saw this patch ticket. I copied the look and feel from the station list cargo type filters to keep things consistent. So just gonna drop it here instead of making a new ticket.

Attachments

• industry_list.patch (36.18 KiB)
• industry_list.png (550.74 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment12157

Alberth wrote:

@NtX: What about other industry sets, eg FIRS or ECS?

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment12158

ntx wrote:

I have limited experience with them, but a quick check with FIRS does reveal that it becomes rather unwieldy, and of course the proper abbreviations for the industries are missing.
Edit: Although the station list also has quite a selection of cargo types there are almost twice as many industries.

Attachments

• firs.jpg (719.27 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455#comment12159

andythenorth closed the ticket.

Reason for closing: Won't implement

Mass closure of patch tickets with no recent commentary. Goal is to reduce patch queue as an experiment to see if it aids faster reviewing and rejection/acceptance (it may not). If this offends you and the patch is maintained and compiles with current trunk, discuss with andythenorth in irc. (andythenorth has no ability to review patches but can re-open tickets).

Thanks for the patch. FWIW, this would be better implemented as a filter dropdown, similar to vehicles filter-by-refittable-cargo.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5455