IRC logs for #openttd.dev on OFTC at 2012-11-16
⏴ go to previous day
04:34:57 *** Knogle has joined #openttd.dev
08:14:53 *** Zuu has joined #openttd.dev
10:25:01 *** Alberth has joined #openttd.dev
10:25:01 *** ChanServ sets mode: +v Alberth
16:36:03 *** Knogle has joined #openttd.dev
16:48:05 *** FLHerne has joined #openttd.dev
17:21:30 *** Zuu has joined #openttd.dev
18:14:55 *** Knogle has joined #openttd.dev
18:24:27 *** Knogle_ has joined #openttd.dev
21:08:15 <Yexo> savegame from FS#5367 says "dbg: [sl] Unknown savegame type, trying to load it as the buggy format" and openttd crashes in lzo1x_decompress in liblzo2.so
21:12:48 <Yexo> hmm, the file consists of zero-bytes only
21:13:28 *** Eagle_Rainbow has joined #openttd.dev
21:13:28 *** ChanServ sets mode: +v Eagle_Rainbow
21:50:59 <planetmaker> nice example screen, Eagle_Rainbow ;-)
21:51:20 <Eagle_Rainbow> nice to hear you being pleased :)
21:52:15 <planetmaker> seems I'm one of the clients being counted therein :-P
21:52:16 <Eagle_Rainbow> and the implementation really was easier with the new edit boxes thingy by frosch than without it... It also decreased the patch size --> a good thing in any case :)
21:53:29 <Yexo> + /* In any case, we need to apply the filter here again. */
21:53:30 <Yexo> + item->filtered_evaluated = false; // If necessary, apply the filter text to this item again.
21:53:34 <Yexo> ^^ which of the two is it?
21:53:37 <planetmaker> + item->filtered_evaluated = false; // If necessary, apply the filter text to this item again.<-- double comment?
21:53:45 <Yexo> does it need to be always reapplied or only when necessary?
21:54:07 <planetmaker> +int NetworkGameListCountFilteredItems()<-- missing doxygen
21:54:21 <planetmaker> +bool NetworkGameListApplyFilter(const char *filter_text, const bool reevaluate_all) <-- also missing doxygen
21:54:28 <Eagle_Rainbow> planetmaker: NetworkGameListCountFilteredItems, nope: it's in the header file...
21:54:31 <Yexo> s/compatability/compatibility/ is fine, but I'd rather not have it in your patch that's supposed to add a feature
21:54:40 <planetmaker> I'm reading sequentially
21:54:56 <Yexo> normal OpenTTD style would be to put the comments with the code
21:54:59 <Eagle_Rainbow> Yexo: will have a look
21:55:00 <planetmaker> send that bug fix to alberth :-) He collects them, I hear
21:55:43 <Yexo> + if (strcmp(filter_text, "") == 0) { <- use StrEmpty(filter_text) instead
21:55:44 <planetmaker> Yexo, there are some parts where they are with headers. It's inconsistent. Unfortunately. I prefer them with code
21:55:56 <Alberth> s/compatability/compatibility/ <-- this one?
21:56:30 <Eagle_Rainbow> Yexo: double comment fixed - it's the // variant
21:56:43 <Alberth> planetmaker: in the no* code, due to separate doxygen generation of the class interfaces
21:56:49 <Yexo> NetworkGameListApplyFilter <- in that function, first while: your indentation is off
21:57:01 <Yexo> you indent as if the if covers everything, but it does not
21:58:25 <Eagle_Rainbow> Doxygen has been moved
21:58:47 <Yexo> filtered_evaluated <- please rename, the name currently implies that the item is both filtered and evaluated or something like that
22:00:00 <Eagle_Rainbow> Alberth: BTW another typo: in src/settings_gui.cpp: s/appropiate/appropriate/
22:00:11 <planetmaker> + uint GetServerIndexByScrolledRow(const uint scroll_pos) const <-- I wonder whether this is not something which could be generalized between filters...
22:00:32 <Yexo> NetworkGameListApplyFilter <- second while also has wrong indentation
22:01:03 <Yexo> two if's in that while could be combined to a single one using &&
22:01:14 <Yexo> if (!reevaluate_all && item_filtered_evaluated) {...}
22:01:43 <Eagle_Rainbow> Yexo: indention - sh*t seems to be something with copy and paste from the old coding... will check
22:02:10 <Yexo> I'd probably use "for (; item != NULL; item = item->next) {" there
22:02:28 <Eagle_Rainbow> indention fixed in NetworkGameListApplyFilter
22:02:29 <Yexo> right now you have "item = item->next;" at two different places, that makes it slightly harder to read
22:03:04 <Yexo> ++count; <- please simply use count++; unless there are good reasons for doing otherwise
22:03:52 <Eagle_Rainbow> s/filtered_evaluated/filter_evaluated/ done
22:03:56 <Yexo> Alberth: s/initialise/initialize/ in network/network_gui.cpp is also one for your collection :)
22:04:35 <planetmaker> Yexo, not sure which is the BE spelling there, really
22:04:44 <Yexo> I was about to say that too, not sure either
22:05:04 <Alberth> I think with the z, but I don't know either
22:05:18 <Yexo> we use initialize 67 times and initialise 17 times in src/, according to a simple grep
22:05:40 <Eagle_Rainbow> very consistent...
22:06:01 <Yexo> +51 / +4 if you also take sub-directories into account
22:06:11 <planetmaker> there's only 1.5 native speakers actively contributing ;-)
22:06:14 <Yexo> to it should most likely be changed to initialize
22:06:19 <Eagle_Rainbow> based on dict.leo.org, initialise is BE and initialize is AE
22:06:29 <Eagle_Rainbow> bit the "z" variant is also used in BE
22:06:59 <planetmaker> so rather initialise
22:07:21 <Yexo> not sure, the z variant seems to be more popular and also valid for BE
22:08:02 <Eagle_Rainbow> and merriam webster (something close to our German "Duden") does not list initialise...
22:08:05 <planetmaker> seems like a case of "doesn't matter" ;-)
22:08:36 <Eagle_Rainbow> and recommends initialize....
22:08:50 <Yexo> sometimes it's too easy to get lost in pointless things :p
22:09:14 <Eagle_Rainbow> Lemme get back to your item = item->next thingy then, Yexo
22:09:38 * Alberth likes the 'z' variant better too
22:11:33 <Alberth> static void InitialiseCrashLog(); <-- haha :)
22:11:35 * Eagle_Rainbow changed "item = item->next;" to the "for" variant
22:13:20 <Eagle_Rainbow> concerning "++count" or "count++": I was use to post-increment and changed habit to pre-increment when I learned that the pre-increment can be optimized better in many cases...
22:13:59 <Eagle_Rainbow> but anyway: you are stating the guidelines here - it's not me :)
22:14:18 <Yexo> it can only be optimized better for (complex) iterator types
22:14:27 <Yexo> for simple integers or pointers there is no difference at all
22:14:41 <Yexo> and post-increment reads more naturally
22:14:53 <Eagle_Rainbow> ok, learned something again :)
22:15:20 <Alberth> Eagle_Rainbow: in general, don't write code for the compiler, write code for fellow human beings
22:15:45 <Alberth> until you have a performance problem
22:16:13 <Eagle_Rainbow> you are taking my argument out of my mouth :) But you are right, here in that case, there is no performance issue at all
22:17:04 <Yexo> I'd like to add a slight point there: if you change code because you have a performance issue, back up your change with profiler data
22:18:05 <Eagle_Rainbow> BTW: I experienced it also from the other side: I have seen hundreds LOCs which were written with bad habits and simply cooked the CPU pointlessly.
22:18:24 <Eagle_Rainbow> So, I am trying to "do it right the first time"...
22:18:41 <Yexo> yeah, that's the other (very bad) end of the spectrum
22:18:48 <Eagle_Rainbow> but for sure: you may exaggerate that as well..
22:19:27 <Eagle_Rainbow> we are getting philosophical again :p
22:19:44 <Yexo> I try to draw the line on algorithmic vs coding changes
22:20:07 <Yexo> a better algorithm is usually ok, harder to read code not unless backed up with data
22:20:51 *** Alberth has left #openttd.dev
22:21:12 <Eagle_Rainbow> so, further things to change for that patch?
22:21:28 <Yexo> dunno, would like to see an updated version first
22:21:37 * Eagle_Rainbow is creating one
22:24:14 <Yexo> don't delete openttd.grf ;)
22:24:40 <Eagle_Rainbow> why is that in?!
22:26:03 <Yexo> compatibility change is still part of your patch
22:26:16 <Yexo> + item->filter_evaluated = false; // If necessary, apply the filter text to this item again. <- comment seems in conflict with the code
22:26:30 <Yexo> the code reads like: "Apply the filter again", not "if necessary", but always
22:27:21 <Eagle_Rainbow> Yexo, well, the coding is right...
22:27:28 <Eagle_Rainbow> Let's think about a better comment
22:27:59 <Eagle_Rainbow> What about: // On next check, if necessary, apply the filter text to this item again
22:27:59 <Yexo> just "// Apply the filter text to this item again." would be fine I think
22:28:26 <Yexo> or "// Mark this item so the filter is applied again."
22:28:33 <Yexo> or remove the comment altogether
22:29:10 <Yexo> + if (StrEmpty(filter_text) == 0) { <- that's wrong, StrEmpty() returns a bool
22:29:19 <Yexo> + if (StrEmpty(filter_text)) { <- use it like that
22:29:38 <Yexo> + // no filter text was specified, thus all entries will be visible <- if on separate line, use /* */
22:29:52 <Eagle_Rainbow> my classical one :p
22:31:05 <Yexo> + StringFilter *sf = new StringFilter(); <- why not simply create it on the stack? Just "StringFilter sf;", then the "delete sf;" later is also unnecessary
22:31:26 <Eagle_Rainbow> I don't like revmoing the "if necessary" one altogether - you could be puzzled here, as you don't think of the filtering here...
22:31:51 <Yexo> combining "if (!reevaluate_all) {" and "if (item->filter_evaluated) {" to a single if-statement still stands
22:32:00 <Yexo> so leave that comment for now
22:32:23 <Yexo> + item = item->next; <- this should be removed
22:32:55 <Eagle_Rainbow> oh man - too many errors tonight...
22:33:11 <Eagle_Rainbow> I should have reread that patch tomorrow once again
22:33:35 <Yexo> extra indentation for comments of struct NetworkGameList is no longer needed
22:33:47 <Yexo> + /* Draw this line. */ <- not a helpful comment
22:34:05 <Yexo> // of 'for' statement <- same for that one
22:34:08 <Eagle_Rainbow> "Draw this line" which was just copied from the original one
22:34:32 <Eagle_Rainbow> "// of 'for' statement" fixed
22:34:46 <Yexo> same for the //of 'switch' statement
22:35:18 <Eagle_Rainbow> well, where I work, these comments for "// of 'switch'..." it's even mandatory...
22:35:23 <Yexo> vscroll_capa <- "cap" is a command short-hand form of "Capacity", "capa" is not
22:36:19 <Yexo> in my opinion the indentation should tell me where the break belongs to
22:36:51 <Yexo> comments like the one you put there are in my eyes just extra noise I have to ignore when trying to understand the actual code
22:37:00 <Eagle_Rainbow> well, I was in a code review with that argument as well, but it was rejected due to the fact of flow complexity....
22:37:25 <Eagle_Rainbow> again seems to be a matter of taste, then...
22:37:58 <Yexo> if (++count >= vscroll_capa) break; // of 'for' statement <- if you change that line to simply "count++;" you could change
22:37:59 <Yexo> for (int i = start_position; i < (int)this->servers.Length(); ++i) {
22:38:14 <Yexo> for (int i = start_position; i < (int)this->servers.Length() && count < vscroll_cap; i++) {
22:38:36 <Yexo> that gets rid of one of the "breaks", which should solve the "flow complexity" :)
22:39:32 <Eagle_Rainbow> that's agreed, then :)
22:40:02 <Yexo> that change is again just a matter of taste, the if in your patch with the break there would be fine for me
22:40:36 <Eagle_Rainbow> well, if you already take your time to look over my patch and we find things that we can improve, then we shalt also do that
22:41:09 <Yexo> I just meant to say: both ways are fine, to me that's not a clear-cut improvement
22:41:25 <Yexo> well, maybe this one is, but it depends on how complex the for would become
22:42:04 <Yexo> + case WID_NG_MATRIX: { // Matrix to show network games available <- if you want to add "available" there, do it like "show available network games"
22:44:53 <Eagle_Rainbow> what about this "@param unselect unselect the currently selected item"
22:45:03 <Eagle_Rainbow> would be another "cleanup activity" which should not go into this feature
22:45:29 <Yexo> yes, the cleanup if fine, but please provide a separate patch
22:45:41 <Eagle_Rainbow> for this single line? :)
22:46:11 <Yexo> you could store it somewhere until you collect a few more lines like that one and provide it at that point
22:46:45 <Yexo> or indeed just provide a patch for that single line
22:47:47 <Eagle_Rainbow> for now: reverted
22:47:53 <Yexo> + const int start_position = this->GetServerIndexByScrolledRow(this->vscroll->GetPosition()); <- we usually don't mark simple data types like "int" as const
22:48:05 <Yexo> in that same line: why int and not uint?
22:48:26 <Yexo> in the next line you have a cast, in the line I quoted you have an implicit cast
22:48:31 <Yexo> both can be avoided by changing to uint
22:49:18 <Eagle_Rainbow> const and uint changed - you are right
22:49:33 <Yexo> + WID_NG_FILTER_LABEL, ///< Label in front of the filter/search edit box <- missing a dot at the end
22:49:34 <Eagle_Rainbow> most likely no problem in practice, though
22:49:50 <Yexo> it's no problem in practice at all, nothing changes
22:50:04 <Yexo> like with most of the coding style ;)
22:52:11 <Eagle_Rainbow> Would you like to have "another round" this time without deleting openttd.grf =:)?
22:54:16 <Yexo> I think most things are ironed out by now
22:54:37 <Yexo> but I'm tired too and don't want to commit now because I'm know I'll overlook things I'd just have to fix tomorrow
22:54:42 <Yexo> better do it right the first time
22:55:09 <Eagle_Rainbow> I have uploaded the most current version.
22:55:18 <Eagle_Rainbow> (same topic on the forum)
22:55:47 * Eagle_Rainbow is tired as well
22:56:07 <Eagle_Rainbow> And that's why I think I will also leave for the day...
22:56:23 <Eagle_Rainbow> Thanks for the review...
23:24:32 *** Supercheese has joined #openttd.dev
continue to next day ⏵