IRC logs for #openttd.dev on OFTC at 2015-11-14
⏴ go to previous day
01:17:36 *** sgtbigman has joined #openttd.dev
05:31:55 *** sgtbigman has joined #openttd.dev
10:23:42 *** Zuu has joined #openttd.dev
12:22:40 <Zuu> It does not add any new GUI. It only changes thet behoviour of online content window when opened from eg. play scenario to only show items of other type than the main type when they have been (auto)selected.
13:32:12 *** Supercheese has joined #openttd.dev
13:32:12 *** ChanServ sets mode: +v Supercheese
13:48:08 *** andythenorth has joined #openttd.dev
13:48:08 *** ChanServ sets mode: +v andythenorth
13:49:50 *** Alberth has joined #openttd.dev
13:49:50 *** ChanServ sets mode: +v Alberth
13:50:14 <Alberth> hihi, not enough activity here to join every time :)
13:50:28 <Zuu> I have it on auto-join :-)
13:50:30 <Alberth> yep, comment fix seems to be correct
13:51:16 <Alberth> auto joins tends to fail if you don't have interwebz when you log in :p
13:53:24 *** frosch123 has joined #openttd.dev
13:53:24 *** ChanServ sets mode: +v frosch123
13:53:55 <andythenorth> someone is coding something
13:54:46 <Alberth> empty line after class NetworkContentListWindow shouldn't be there
13:54:54 <Zuu> Someone is making a full checkout of svn because his old was of too old format or something.
13:56:06 <frosch123> svn upgrade would be easier :p
13:56:20 <Zuu> That asumes you know svn these days
13:56:35 <frosch123> it told me on the command line, to run that command
13:57:02 <Alberth> yeah, me too, except then it killed my working-copy :p
13:57:27 <Alberth> perhaps now is a good time to re-align all those variable comments?
13:58:16 <Zuu> Possible, but preferable in a second patch I suppose
13:58:17 <Alberth> TagNameFilter comment is out of date, probably
14:00:01 <frosch123> hmm, i thought we already had such filter
14:00:20 <Alberth> hmm, no, code is just confusing in not using the content type
14:02:20 <Alberth> last condition value in TypeOrSelectedFilter can be simply returned
14:02:36 <frosch123> hmm, ah, the new thing is that it still shows dependencies, also when only showing one type otherwise, right?
14:02:44 <Zuu> frosch123: There is a filter on what content type to include when building the list. Or actuallly in what content _network_content_client provides. What the patch does is hiding content of wrong type unless it has been (auto)selected.
14:03:31 <frosch123> yeah, trunk already can do that for heightmaps, because they have no dependencies
14:04:04 <frosch123> and for newgrf, because they only depend on newgrf
14:04:40 <Zuu> If you open online content in trunk from play scenario, you get all sorts of content but scenarios in top (in English)
14:05:01 <frosch123> hmm, but if i open it from the gs/ai gui, i get gs/ai and the "split" scenario
14:05:25 <frosch123> are you sure it doesn't already work?
14:05:33 <Zuu> Split may still be using the cyclic dependency "hack" and not regular musa depnedencies that only go in one way.
14:05:34 <Alberth> if (this->filter_data.type != CONTENT_TYPE_END) { <-- do you need this, given if (filter.type == CONTENT_TYPE_END) return true; ?
14:07:13 <Alberth> /* comment lines like these should be normal english sentences with uppercase and a dot */
14:07:46 <frosch123> bananas code is such a mystery to me
14:08:04 <frosch123> fios_gui calls ShowNetworkContentListWindow(NULL, CONTENT_TYPE_SCENARIO); , why does it show other stuff?
14:08:29 <frosch123> did someone make a hack on the server side to also return other stuff?
14:08:47 <Alberth> or CONTENT_TYPE_END <-- Add '#' , like "or #CONTENT_TYPE_END" to link to the definition
14:08:59 <frosch123> oh wait, it doesn't display all stuff
14:10:46 <Alberth> !this->filter_data.string_filter.IsEmpty() || this->filter_data.type != CONTENT_TYPE_END <-- fold that in a function/method?
14:11:50 <Zuu> <Alberth> if (this->filter_data.type != CONTENT_TYPE_END) { <-- do you need this, given if (filter.type == CONTENT_TYPE_END) return true; ? <---- it is only needed for optimization. With this check, we do not even loop all content and return true.
14:12:36 <Alberth> + * when a type other than CONTENT_TYPE_END is given, <-- Missing "W", also, is it part of the @param above it? Please indent it or make it clear otherwise
14:13:34 <Zuu> It is part of the param. I'll indent it.
14:15:05 <Alberth> ForceRebuild implies ForceResort? If so, you can check for rebuild first
14:15:19 <frosch123> haha, that patch adds the first use of GUIList::SetFilterType :p
14:15:39 <Zuu> frosch123: Yeah I found it interesting it was not used yet. :-)
14:16:09 <frosch123> well, i never used it, because it made no sense to me :)
14:16:26 <frosch123> i always put everything into a single fitler function, even when filtering by multiple criterions
14:16:49 <frosch123> i couldn't see any sense in setting SetFilterType
14:17:39 <Zuu> Alberth: when indenting param text, do you just indent by two(?) spaces? or to get a nice left margin that follow with the first word after @param on the first row?
14:18:14 <Zuu> frosch123: I was wondering if I should just put it in one function too. But then we have this SetFilterType facility.. :-)
14:18:26 <Alberth> + this->UpdateFilterState(); <-- do 'ScrollToSelected' above it, and then potentially force a rebuild? isn't it better the other way around?
14:19:07 <frosch123> + void UpdateFilterState() { <- missing \n
14:19:16 <Alberth> Zuu: don't think we have a code style for that, but not against the left margin at least would be great
14:19:26 <Zuu> A forced rebuild doesn't actually hapen directly. It sets a bit. It is first at OnInvalidateData, the list is actually rebuilt.
14:21:35 <Zuu> Hmm BulidContentList will scroll to selected, so I could UpdateFilterState() first, then check if the rebuild bit is set, and only if not, scroll to selected.
14:23:35 <frosch123> how about adding a bool parameter "void UpdateFilterState(bool editbox_changed)", and also call it from OnEditboxChanged?
14:23:54 <frosch123> that woudl save deduplication of the SetFilterState condition
14:24:17 <Zuu> I updated the patch on /~zuu/
14:24:23 <frosch123> possibly it could also be called from the constructor
14:25:45 <Zuu> It can as well be used in OnEditboxChanged. It would reduce duplication of SetFilterState, but may call ForceRebuild() twice, but that only set a bit so it may not be a big issue.
14:28:06 <frosch123> that's what i mean with the bool parameter
14:28:15 <frosch123> it would force the update, even if the filterstate does not change
14:28:18 <frosch123> same in constructor
14:30:55 <Zuu> But perhaps rename editbox_changed into force_rebuild if it is used in the constructor too.
14:32:45 <Zuu> Or UpdateFilterState could just return a bool if fiter state was updated. Will duplicate a bit more code to rebuild list, but make it more clear what UpdateFilterState does from its name.
14:33:34 <Zuu> Eg. remove the whole rebuilding from UpdateFilterState.
14:40:00 <Zuu> Hmm that actually made sense in code. I think only two places need to check the return value. One need to force rebuild + invalidate and the other only force rebuild.
14:47:34 <frosch123> + if(new_state != old_params.state) { <- missing space after "if"
14:50:05 <Zuu> too little coffe or so :-)
14:51:49 <Alberth> /me gives another coffee to zuu
14:52:14 <Zuu> I think actually I should get out and catch some daylight before its too late in an hour or so. :-)
16:17:59 <Zuu> I'm temped to close https://bugs.openttd.org/task/5470 with my patch. It do put forward an issue about having multiple versions of libraries visible in the main list due to them being dependencis, which my patch doesn't address. On the other hand, do we want to hide things from the main list?
16:22:26 <Zuu> Looking at network_content.cpp it seems the server do just pass back old versions at its own descision. I'm not sure we can detect on the client at the moment which content is 'old' in this sense. On the other hand that doesn't make the FS task invalid just because it is hard to solve.
16:28:23 <Zuu> Hmm.. the user screenshots indicate he/she is using the main list. And that is not dealed with in my patch. So I leave it open.
16:30:24 <Rubidium> the server doesn't make a decision, in the first batch it sends everything that is marked "active" in the database
16:34:23 <Rubidium> in the client on SERVER_INFO it will call CheckDependencyState which will call DownloadContentInfo to download the content info for the dependencies
16:34:33 <Rubidium> so all the dependency handling is client side
16:35:19 <Zuu> So then cilent could set a flag on those dependencies it receive after having received active content.
16:36:58 <Rubidium> there's a requested instance variable that might be useful, but I'm not quite sure
16:41:03 <Zuu> A problem may be how to when receiving content info data from server know how/why it was requested. A global state may be unreliable.
16:41:26 <Rubidium> the server doesn't tell you
16:42:21 <Rubidium> and possibly you can't even really know in the client yourself
16:42:49 <Rubidium> since it's just a stream of contentinfos coming from the server
16:42:57 <Zuu> I found the code that make the requests. In JavaScript it would be easy, but here it may need to be passed along to the server and back again, unless there request key/index passed along with the request and passed back.
16:44:10 <Rubidium> and to make matters worse, if the first thing that is returned in the "content list" has dependencies that are further along in the list, then it will just request them twice
16:44:52 <Rubidium> so the "requests" variable might be misleading
16:46:00 <Zuu> I think I will pass on this issue. The patch deal with content list limited to one type, but is of no help for the general content list.
17:17:50 *** ChanServ sets mode: +o DorpsGek
17:18:02 *** DorpsGek sets mode: +v Eearslya
18:50:28 *** sgtbigman has joined #openttd.dev
20:01:45 *** Alberth has left #openttd.dev
21:04:12 *** sgtbigman has joined #openttd.dev
22:53:24 *** andythenorth has left #openttd.dev
continue to next day ⏵