Log for on 16th November 2012:
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
06:58:19  *** Knogle has quit IRC
07:54:41  *** Supercheese has quit IRC
08:14:41  *** Zuu has joined
08:14:42  *** ChanServ sets mode: +v Zuu
08:38:20  *** Zuu has quit IRC
10:24:49  *** Alberth has joined
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
16:47:53  *** FLHerne has joined
17:21:18  *** Zuu has joined
17:21:18  *** ChanServ sets mode: +v Zuu
18:13:33  *** Knogle has quit IRC
18:14:43  *** Knogle has joined
18:24:16  *** Knogle_ has joined
18:26:48  *** Knogle has quit IRC
18:45:11  *** DorpsGek changes topic to "OpenTTD Dev Channel || Latest SVN: r24752 || Logs: || 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
21:09:15  <Yexo>
21:12:37  <Yexo> hmm, the file consists of zero-bytes only
21:13:16  *** Eagle_Rainbow has joined
21:13:16  *** ChanServ sets mode: +v Eagle_Rainbow
21:14:34  <Eagle_Rainbow> good eveinng
21:15:59  <Eagle_Rainbow> <-- 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, 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
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>
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

Powered by YARRSTE version: svn-trunk