Times are UTC Toggle Colours
01:45:32 *** FLHerne has quit IRC 04:34:35 *** Knogle has quit IRC 04:34:46 *** Knogle has joined #openttd.dev 06:58:19 *** Knogle has quit IRC 07:54:41 *** Supercheese has quit IRC 08:14:41 *** Zuu has joined #openttd.dev 08:14:42 *** ChanServ sets mode: +v Zuu 08:38:20 *** Zuu has quit IRC 10:24:49 *** Alberth has joined #openttd.dev 10:24:49 *** ChanServ sets mode: +v Alberth 14:33:05 <Belugas> hello 14:57:13 <Alberth> hello sir B 15:01:25 <Belugas> hi A 15:01:28 <Belugas> ;) 15:06:01 <planetmaker> hello A&B :D 15:26:01 <Belugas> hi P 15:26:07 <Belugas> mmhh.. i need to P 16:35:51 *** Knogle has joined #openttd.dev 16:47:53 *** FLHerne has joined #openttd.dev 17:21:18 *** Zuu has joined #openttd.dev 17:21:18 *** ChanServ sets mode: +v Zuu 18:13:33 *** Knogle has quit IRC 18:14:43 *** Knogle has joined #openttd.dev 18:24:16 *** Knogle_ has joined #openttd.dev 18:26:48 *** Knogle has quit IRC 18:45:11 *** DorpsGek changes topic to "OpenTTD Dev Channel || Latest SVN: r24752 || Logs: http://webster.openttdcoop.org/?channel=openttd.dev || Voice (talk-right) upon request via #openttd; make sure you are registered to NickServ before asking" 20:15:58 *** FLHerne has quit IRC 21:08:03 <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:09:15 <Yexo> http://paste.openttdcoop.org/show/1936/ 21:12:37 <Yexo> hmm, the file consists of zero-bytes only 21:13:16 *** Eagle_Rainbow has joined #openttd.dev 21:13:16 *** ChanServ sets mode: +v Eagle_Rainbow 21:14:34 <Eagle_Rainbow> good eveinng 21:15:59 <Eagle_Rainbow> http://www.tt-forums.net/viewtopic.php?f=33&t=59329&p=1054303#p1054303 <-- updates the multiplayer filter edit box to the newest revision of trunk making use of frosch's patches from Wednesday 21:50:47 <planetmaker> nice example screen, Eagle_Rainbow ;-) 21:51:00 <Eagle_Rainbow> thanks 21:51:08 <Eagle_Rainbow> nice to hear you being pleased :) 21:52:03 <planetmaker> seems I'm one of the clients being counted therein :-P 21:52:04 <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:52:21 <planetmaker> yup 21:53:17 <Yexo> + /* In any case, we need to apply the filter here again. */ 21:53:18 <Yexo> + item->filtered_evaluated = false; // If necessary, apply the filter text to this item again. 21:53:22 <Yexo> ^^ which of the two is it? 21:53:25 <planetmaker> + item->filtered_evaluated = false; // If necessary, apply the filter text to this item again.<-- double comment? 21:53:33 <Yexo> does it need to be always reapplied or only when necessary? 21:53:55 <planetmaker> +int NetworkGameListCountFilteredItems()<-- missing doxygen 21:54:09 <planetmaker> +bool NetworkGameListApplyFilter(const char *filter_text, const bool reevaluate_all) <-- also missing doxygen 21:54:16 <Eagle_Rainbow> planetmaker: NetworkGameListCountFilteredItems, nope: it's in the header file... 21:54:19 <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:20 <planetmaker> ah... below 21:54:28 <planetmaker> I'm reading sequentially 21:54:44 <Yexo> normal OpenTTD style would be to put the comments with the code 21:54:47 <Eagle_Rainbow> Yexo: will have a look 21:54:48 <planetmaker> send that bug fix to alberth :-) He collects them, I hear 21:55:31 <Yexo> + if (strcmp(filter_text, "") == 0) { <- use StrEmpty(filter_text) instead 21:55:32 <planetmaker> Yexo, there are some parts where they are with headers. It's inconsistent. Unfortunately. I prefer them with code 21:55:44 <Alberth> s/compatability/compatibility/ <-- this one? 21:55:49 <Yexo> Alberth: yes 21:55:49 <planetmaker> Alberth, yes 21:56:18 <Eagle_Rainbow> Yexo: double comment fixed - it's the // variant 21:56:31 <Alberth> planetmaker: in the no* code, due to separate doxygen generation of the class interfaces 21:56:37 <Yexo> NetworkGameListApplyFilter <- in that function, first while: your indentation is off 21:56:49 <Yexo> you indent as if the if covers everything, but it does not 21:57:17 <Eagle_Rainbow> StrEmpty fixed 21:58:13 <Eagle_Rainbow> Doxygen has been moved 21:58:35 <Yexo> filtered_evaluated <- please rename, the name currently implies that the item is both filtered and evaluated or something like that 21:58:49 <Yexo> try "filter_evaluated" 21:59:11 <Yexo> or "filter_applied" 21:59:48 <Eagle_Rainbow> Alberth: BTW another typo: in src/settings_gui.cpp: s/appropiate/appropriate/ 21:59:59 <planetmaker> + uint GetServerIndexByScrolledRow(const uint scroll_pos) const <-- I wonder whether this is not something which could be generalized between filters... 22:00:05 <Alberth> Eagle_Rainbow: ok 22:00:20 <Yexo> NetworkGameListApplyFilter <- second while also has wrong indentation 22:00:51 <Yexo> two if's in that while could be combined to a single one using && 22:01:02 <Yexo> if (!reevaluate_all && item_filtered_evaluated) {...} 22:01:31 <Eagle_Rainbow> Yexo: indention - sh*t seems to be something with copy and paste from the old coding... will check 22:01:58 <Yexo> I'd probably use "for (; item != NULL; item = item->next) {" there 22:02:17 <Eagle_Rainbow> indention fixed in NetworkGameListApplyFilter 22:02:17 <Yexo> right now you have "item = item->next;" at two different places, that makes it slightly harder to read 22:02:52 <Yexo> ++count; <- please simply use count++; unless there are good reasons for doing otherwise 22:03:40 <Eagle_Rainbow> s/filtered_evaluated/filter_evaluated/ done 22:03:44 <Yexo> Alberth: s/initialise/initialize/ in network/network_gui.cpp is also one for your collection :) 22:04:23 <planetmaker> Yexo, not sure which is the BE spelling there, really 22:04:31 <planetmaker> the other is AE 22:04:32 <Yexo> I was about to say that too, not sure either 22:04:52 <Alberth> I think with the z, but I don't know either 22:05:06 <Yexo> we use initialize 67 times and initialise 17 times in src/, according to a simple grep 22:05:11 <planetmaker> :D 22:05:28 <Eagle_Rainbow> very consistent... 22:05:49 <Yexo> +51 / +4 if you also take sub-directories into account 22:05:59 <planetmaker> there's only 1.5 native speakers actively contributing ;-) 22:06:02 <Yexo> to it should most likely be changed to initialize 22:06:07 <Eagle_Rainbow> based on dict.leo.org, initialise is BE and initialize is AE 22:06:17 <Eagle_Rainbow> bit the "z" variant is also used in BE 22:06:47 <planetmaker> so rather initialise 22:07:09 <Yexo> not sure, the z variant seems to be more popular and also valid for BE 22:07:50 <Eagle_Rainbow> and merriam webster (something close to our German "Duden") does not list initialise... 22:07:53 <planetmaker> seems like a case of "doesn't matter" ;-) 22:08:24 <Eagle_Rainbow> and recommends initialize.... 22:08:27 <Yexo> yes :) 22:08:38 <Yexo> sometimes it's too easy to get lost in pointless things :p 22:09:02 <Eagle_Rainbow> Lemme get back to your item = item->next thingy then, Yexo 22:09:20 <Yexo> that's not a big deal 22:09:26 * Alberth likes the 'z' variant better too 22:11:21 <Alberth> static void InitialiseCrashLog(); <-- haha :) 22:11:23 * Eagle_Rainbow changed "item = item->next;" to the "for" variant 22:13:08 <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:29 *** Zuu has quit IRC 22:13:48 <Eagle_Rainbow> but anyway: you are stating the guidelines here - it's not me :) 22:14:06 <Yexo> it can only be optimized better for (complex) iterator types 22:14:15 <Yexo> for simple integers or pointers there is no difference at all 22:14:29 <Yexo> and post-increment reads more naturally 22:14:41 <Eagle_Rainbow> ok, learned something again :) 22:14:45 * Alberth likes x++ better too 22:15:08 <Alberth> Eagle_Rainbow: in general, don't write code for the compiler, write code for fellow human beings 22:15:33 <Alberth> until you have a performance problem 22:16:01 <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:16:02 <Eagle_Rainbow> => changed 22:16:52 <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:17:53 <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:12 <Eagle_Rainbow> So, I am trying to "do it right the first time"... 22:18:29 <Yexo> yeah, that's the other (very bad) end of the spectrum 22:18:36 <Eagle_Rainbow> but for sure: you may exaggerate that as well.. 22:19:15 <Eagle_Rainbow> we are getting philosophical again :p 22:19:32 <Yexo> I try to draw the line on algorithmic vs coding changes 22:19:55 <Yexo> a better algorithm is usually ok, harder to read code not unless backed up with data 22:19:59 <Yexo> but yes :) 22:20:37 <Alberth> good night 22:20:39 *** Alberth has left #openttd.dev 22:21:00 <Eagle_Rainbow> so, further things to change for that patch? 22:21:16 <Yexo> dunno, would like to see an updated version first 22:21:25 * Eagle_Rainbow is creating one 22:23:35 <Eagle_Rainbow> http://www.tt-forums.net/download/file.php?id=165776 22:24:02 <Yexo> don't delete openttd.grf ;) 22:24:15 <Eagle_Rainbow> wow! 22:24:28 <Eagle_Rainbow> why is that in?! 22:25:51 <Yexo> compatibility change is still part of your patch 22:26:04 <Yexo> + item->filter_evaluated = false; // If necessary, apply the filter text to this item again. <- comment seems in conflict with the code 22:26:18 <Yexo> the code reads like: "Apply the filter again", not "if necessary", but always 22:27:09 <Eagle_Rainbow> Yexo, well, the coding is right... 22:27:16 <Eagle_Rainbow> Let's think about a better comment 22:27:47 <Eagle_Rainbow> What about: // On next check, if necessary, apply the filter text to this item again 22:27:47 <Yexo> just "// Apply the filter text to this item again." would be fine I think 22:28:14 <Yexo> or "// Mark this item so the filter is applied again." 22:28:21 <Yexo> or remove the comment altogether 22:28:58 <Yexo> + if (StrEmpty(filter_text) == 0) { <- that's wrong, StrEmpty() returns a bool 22:29:07 <Yexo> + if (StrEmpty(filter_text)) { <- use it like that 22:29:21 <Eagle_Rainbow> got it 22:29:24 <Eagle_Rainbow> fixed 22:29:26 <Yexo> + // no filter text was specified, thus all entries will be visible <- if on separate line, use /* */ 22:29:40 <Eagle_Rainbow> my classical one :p 22:29:51 <Eagle_Rainbow> fixed 22:30:53 <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:14 <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:39 <Yexo> combining "if (!reevaluate_all) {" and "if (item->filter_evaluated) {" to a single if-statement still stands 22:31:48 <Yexo> so leave that comment for now 22:32:11 <Yexo> + item = item->next; <- this should be removed 22:32:43 <Eagle_Rainbow> oh man - too many errors tonight... 22:32:59 <Eagle_Rainbow> I should have reread that patch tomorrow once again 22:33:23 <Yexo> extra indentation for comments of struct NetworkGameList is no longer needed 22:33:35 <Yexo> + /* Draw this line. */ <- not a helpful comment 22:33:53 <Yexo> // of 'for' statement <- same for that one 22:33:56 <Eagle_Rainbow> "Draw this line" which was just copied from the original one 22:34:20 <Eagle_Rainbow> "// of 'for' statement" fixed 22:34:34 <Yexo> same for the //of 'switch' statement 22:35:06 <Eagle_Rainbow> well, where I work, these comments for "// of 'switch'..." it's even mandatory... 22:35:11 <Yexo> vscroll_capa <- "cap" is a command short-hand form of "Capacity", "capa" is not 22:35:39 <Eagle_Rainbow> capa fixed 22:36:07 <Yexo> in my opinion the indentation should tell me where the break belongs to 22:36:39 <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:36:48 <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:13 <Eagle_Rainbow> again seems to be a matter of taste, then... 22:37:20 <Yexo> as with a lot of things 22:37:46 <Yexo> if (++count >= vscroll_capa) break; // of 'for' statement <- if you change that line to simply "count++;" you could change 22:37:47 <Yexo> for (int i = start_position; i < (int)this->servers.Length(); ++i) { 22:38:02 <Yexo> for (int i = start_position; i < (int)this->servers.Length() && count < vscroll_cap; i++) { 22:38:24 <Yexo> that gets rid of one of the "breaks", which should solve the "flow complexity" :) 22:39:20 <Eagle_Rainbow> that's agreed, then :) 22:39:50 <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:24 <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:40:57 <Yexo> I just meant to say: both ways are fine, to me that's not a clear-cut improvement 22:41:13 <Yexo> well, maybe this one is, but it depends on how complex the for would become 22:41:53 <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:43:33 <Eagle_Rainbow> done 22:44:41 <Eagle_Rainbow> what about this "@param unselect unselect the currently selected item" 22:44:51 <Eagle_Rainbow> would be another "cleanup activity" which should not go into this feature 22:45:17 <Yexo> yes, the cleanup if fine, but please provide a separate patch 22:45:29 <Eagle_Rainbow> for this single line? :) 22:45:59 <Yexo> you could store it somewhere until you collect a few more lines like that one and provide it at that point 22:46:33 <Yexo> or indeed just provide a patch for that single line 22:47:35 <Eagle_Rainbow> for now: reverted 22:47:41 <Yexo> + const int start_position = this->GetServerIndexByScrolledRow(this->vscroll->GetPosition()); <- we usually don't mark simple data types like "int" as const 22:47:53 <Yexo> in that same line: why int and not uint? 22:48:14 <Yexo> in the next line you have a cast, in the line I quoted you have an implicit cast 22:48:19 <Yexo> both can be avoided by changing to uint 22:49:06 <Eagle_Rainbow> const and uint changed - you are right 22:49:21 <Yexo> + WID_NG_FILTER_LABEL, ///< Label in front of the filter/search edit box <- missing a dot at the end 22:49:22 <Eagle_Rainbow> most likely no problem in practice, though 22:49:38 <Yexo> it's no problem in practice at all, nothing changes 22:49:52 <Yexo> like with most of the coding style ;) 22:50:22 <Eagle_Rainbow> dot fixed 22:51:59 <Eagle_Rainbow> Would you like to have "another round" this time without deleting openttd.grf =:)? 22:53:43 <Yexo> not tonight 22:53:54 <Eagle_Rainbow> ok 22:54:04 <Yexo> I think most things are ironed out by now 22:54:25 <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:30 <Yexo> better do it right the first time 22:54:47 <Eagle_Rainbow> :P 22:54:51 <Eagle_Rainbow> That's ok... 22:54:57 <Eagle_Rainbow> I have uploaded the most current version. 22:55:06 <Eagle_Rainbow> (same topic on the forum) 22:55:35 * Eagle_Rainbow is tired as well 22:55:55 <Eagle_Rainbow> And that's why I think I will also leave for the day... 22:56:11 <Eagle_Rainbow> Thanks for the review... 22:57:41 <Eagle_Rainbow> good night 22:57:45 *** Eagle_Rainbow has quit IRC 23:24:20 *** Supercheese has joined #openttd.dev