#gemrb@irc.freenode.net logs for 13 Jun 2010 (GMT)

Archive Today Yesterday Tomorrow
GemRB homepage

[01:02:44] <-- Maighstir_laptop has left IRC (Quit: Maighstir_laptop)
[01:27:21] <-- raevol has left IRC (Quit: Leaving.)
[05:04:31] <tomprince> fuzzie: One thought re: magic gui functions.
[05:06:15] <tomprince> Right now, we look them up in the context of whatever python module we happen to be running.
[05:07:32] <tomprince> It seems like it might make more sense to have a particular module that we grab them all from.
[05:18:54] <tomprince> or.cz/guicallback2: This is the work in progress patch series that makes event handlers be python objects.
[06:02:40] <tomprince> I havent thought deeply about what happens when we need to call the magic functions from python as well.
[06:21:37] <-- Gekz has left IRC (Changing host)
[06:21:37] --> Gekz has joined #GemRb
[06:27:18] --> rb` has joined #GemRb
[06:27:30] <rb`> hello
[07:06:02] <-- Gekz has left IRC (Ping timeout: 245 seconds)
[07:13:05] <-- rb` has left IRC (Quit: leaving)
[07:18:59] --> Gekz has joined #GemRb
[07:19:09] <-- Gekz has left IRC (Changing host)
[07:19:09] --> Gekz has joined #GemRb
[07:34:16] --> lynxlynxlynx has joined #GemRb
[07:34:16] --- ChanServ gives channel operator status to lynxlynxlynx
[07:52:01] <fuzzie> tomprince: the "look them up in the context of the current module" is deliberate.
[07:52:18] <fuzzie> it means you can replace behaviour per-screen.
[07:52:43] <fuzzie> don't know if it makes sense.
[07:54:16] <fuzzie> but it seems a nicer api than forcing them to a specific module.
[08:01:33] <fuzzie> re the branch: it looks okay, although the note to remove the EventHandler typedef seems not so good
[08:02:13] <fuzzie> and there's some fiddling with cmake in the middle
[08:04:15] <fuzzie> but the whole thing seems like a hack, really
[08:04:54] <fuzzie> so aproper review might have to wait until it's tidier
[08:05:08] <fuzzie> but please please don't scatter something unreadable like Holder<ScriptEngine::Callable> everywhere. :P
[08:05:25] <fuzzie> the Holder makes the thing unpredictable magic anyway, so i don't see a typedef makes it worse
[08:06:20] <fuzzie> also g'morning.
[08:12:08] <lynxlynxlynx> i agree
[08:13:03] <lynxlynxlynx> “Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.” – Brian W. Kernighan
[08:48:23] --> raevol has joined #GemRb
[08:53:43] --> cubathy has joined #GemRb
[09:17:29] <-- raevol has left IRC (Quit: Leaving.)
[09:24:55] <Lightkey> nice one :-D
[11:10:02] <pupnik> if you write the code as cleverly as possible you have been cheating your employer out of time too
[11:12:53] <-- Gekz has left IRC (Read error: Connection reset by peer)
[11:17:48] --> Gekz has joined #GemRb
[12:10:11] <-- cubathy has left IRC (Ping timeout: 260 seconds)
[12:35:12] --> Gekz_ has joined #GemRb
[12:52:54] <-- fuzzie has left IRC (Ping timeout: 260 seconds)
[12:53:29] --> fuzzie has joined #GemRb
[12:57:45] <tomprince> Well, it may be deliberate, but we don't use the ability to redefine it anywhere. This is what we use it for: http://pastebin.com/B1tuRQ7R
[13:03:18] <fuzzie> well, yes, but that's a bug
[13:03:39] <fuzzie> it might well be better handled as you say, with a single function which checks which screen is open
[13:05:35] <fuzzie> but "we don't use this functionality right now" is a really ridiculous argument given how unfinished gemrb is :)
[13:06:40] <fuzzie> i can't really tell much from that list, because i imagine a lot of that is just making up for our lack of keybindings
[13:07:27] <-- Gekz_ has left IRC (Changing host)
[13:07:27] --> Gekz_ has joined #GemRb
[13:08:26] <tomprince> I wonder if anywhere where we need different behavior for different screens, would it be better not to have it as a magic name, but have it be settable from the python side?
[13:08:44] <fuzzie> why?
[13:09:41] <fuzzie> i mean, i am honestly curious
[13:14:52] <fuzzie> i can see a good argument for the core-called functions being in a seperate module, because it makes the API a lot clearer
[13:15:13] <tomprince> It seems like having multiple definitions of magic functions would be confusing. Particularily, since they pulled in by import statements.
[13:15:19] <fuzzie> but i can't see one for magic names being settable from the python side
[13:16:01] <tomprince> Well, I wasn't suggesting that we make any of the current ones settable.
[13:16:41] <tomprince> Just that if we run into a need to call something different, on different screens, from C++, that what we call should be settable from python.
[13:16:51] <fuzzie> why?
[13:17:18] <fuzzie> i mean, we *do* need to do that, but surely if you're going to insist the python side do work, it might as well just have an if statement in there
[13:18:00] <tomprince> I guess that depends on how different the work is.
[13:18:53] <fuzzie> well, opening dialogue/containers/etc needs the current window to close itself down and open the main screen again
[13:19:06] <fuzzie> and the keybindings need to do special things, but i really think that should all be python-side
[13:19:42] <fuzzie> and the control stuff needs to be smarter
[13:19:47] <fuzzie> but i suspect it could all be handled via 'if'
[13:20:51] <fuzzie> just a bit worried that we're going to end up with inflexibility built into APIs, as usual
[13:24:23] <tomprince> Well, a bit of inflexibility isn't a bad thing, if it doesn't provide useful functionality / prevents bad design.
[13:25:23] <tomprince> I don't know if this is such a case.
[13:26:00] <fuzzie> sure, so i discussed before how the sensible way to approach this would be to refactor the python side first, and then it would become apparent what was good/bad
[13:26:22] <fuzzie> and that is what lynxlynxlynx seems to have been continuing to work on
[13:29:30] <lynxlynxlynx> i'm mostly killing nplication
[13:30:03] <fuzzie> imo it's impossible to refactor the guiscripts unless we kill most of the duplication first
[13:30:24] <fuzzie> i spent enough time just copying bugfixes between scripts, i really wouldn't want to do that with bigger changes
[13:31:01] <lynxlynxlynx> do any of you two have bg2 handy?
[13:31:13] <tomprince> Not the original.
[13:31:51] <fuzzie> not now, but i could look something up later
[13:32:20] <lynxlynxlynx> it's ok, i see what's wrong
[13:32:58] <tomprince> About callbacks, would it make sense to move Callback out of ScriptEngine?
[13:33:18] <tomprince> And if we keep the typedef, it should probably be moved inside Control.
[13:34:36] <fuzzie> sure, that makes sense, if there's no serious uses of the callbacks elsewhere
[13:35:13] --> budlust has joined #GemRb
[13:35:22] <fuzzie> i mean, re: the typedef
[13:35:34] <fuzzie> the callback thing i have no idea about
[13:38:07] <tomprince> Well, there shouldn't be a typedef at global scope in Control.h, for something in ScriptEngine.h
[13:38:38] <tomprince> If you think we should use the typedef everywhere, then perhaps a slightly more generic name, and moving it into ScriptEngine?
[13:39:01] <fuzzie> well, then you have a silly long typedef
[13:39:12] <CIA-23> GemRB: 03lynxlupodian * ra95b71ae6c41 10gemrb/gemrb/GUIScripts/ (14 files in 6 dirs): removed some redundant guiscript includes (thanks to bin/snakefood)
[13:39:13] <CIA-23> GemRB: 03lynxlupodian * r34025d741b06 10gemrb/gemrb/GUIScripts/bg2/GUICG2.py: bg2::cg: added a missing include
[13:39:26] <fuzzie> but from a glance it looked like there were no serious uses of the typedef outside of Control
[13:39:32] <tomprince> ScriptEngine.h, I meant.
[13:39:53] <fuzzie> it isn't possible to avoid forcing everything to include ScriptEngine.h?
[13:40:14] <fuzzie> i guess it's tiny though
[13:40:24] <fuzzie> so, that sounds fine too
[13:40:47] <tomprince> Well, that was one reason for moving Callback out of ScriptEngine, that way it could be in its onw header.
[13:41:52] <fuzzie> well, they're all design decisions that seem to have some argument either way, so i think it's down to your choice..
[13:43:27] <tomprince> But I am not sure which would be better ....
[13:44:24] <fuzzie> well, me neither :) but your arguments make sense
[13:50:22] <-- Gekz_ has left IRC (Quit: Leaving)
[13:50:33] <-- Gekz has left #GemRb ("Leaving")
[14:26:40] --> Maighstir_laptop has joined #GemRb
[15:12:08] --> tombhadAC has joined #GemRb
[15:12:24] <lynxlynxlynx> snakefood is pretty useless apart from this include check
[15:12:33] <-- tombhadAC has left IRC (Remote host closed the connection)
[16:39:19] <tomprince> We set ReturnToGame as an EventHandler and never define it.
[16:39:42] --> pupnik_ has joined #GemRb
[16:41:02] <-- anji has left IRC (Quit: ZNC)
[16:41:41] <tomprince> Is this how CloseOtherWindow works:
[16:42:14] <tomprince> 1. Some function calls CloseOtherWindow with itself as parameter.
[16:42:33] --> anji has joined #GemRb
[16:43:08] <tomprince> 2. CloseOtherWindow has a saved reference to some other window opening function and calls it.
[16:43:12] <-- pupnik has left IRC (Ping timeout: 252 seconds)
[16:43:52] <fuzzie> yes.
[16:43:55] <tomprince> 3. That function calls CloseOtherWindow with itself.
[16:44:24] <tomprince> 4. CloseOtherWindow sees that the saved ref and the param are the same, so returns 0.
[16:44:40] <tomprince> 5. The function does the window closing stuff.
[16:45:05] <tomprince> 6. CloseOtherWindow returns 0, causing the original function to run its normal path.
[16:45:15] <tomprince> s/0/1/ in 4.
[16:45:18] <fuzzie> well, .. yes, that :)
[16:45:55] <tomprince> That just seems .... insane.
[16:46:17] <fuzzie> It is.
[16:47:04] <fuzzie> I've had to patch up quite a few of the callers because they've ended up broken, but never found the time to make it work properly.
[16:47:34] <fuzzie> Not sure what a better design would be, but "having seperate open and close functions" would certainly be at the top of my list.
[16:48:52] <fuzzie> I imagine the design is because you want a single function for each menu button, but a ToggleInventoryScreen function or whatever would be far more sensible for that.
[16:49:04] <tomprince> Yes. If you wanted to stay close to the current design, just having separate functions, and calling CloseOtherWindow with the current window close function.
[16:49:22] <tomprince> Then CloseOtherWindow just uncondtionally calls the saved function, then updates the saved function.
[16:49:27] <lynxlynxlynx> a KISS design and i'll be happy
[16:49:29] <fuzzie> i spy a "<fuzzie> if you would like another challenge after you are done with your long long list, CloseOtherWindow is the bane of
[16:49:32] <fuzzie> my existance" in my irc log :)
[16:49:41] <tomprince> :)
[16:49:46] <fuzzie> don't know if this came from that, but you can see why i suggested
[16:51:08] <tomprince> No, this didn't come from that. I was trying to figure out why we try to call the non-existant ReturnToGame.
[16:51:11] <fuzzie> for the messiest case which actually works, i think look at iwd2/GUIMA.py
[16:51:50] <fuzzie> oh, right
[16:52:02] <fuzzie> that's the hack where we don't actually need a function, we just need the close handler called?
[16:54:04] <tomprince> I think maybe..
[16:56:38] <lynxlynxlynx> yep
[17:05:14] <fuzzie> one of those things i get confused by every time i see it
[17:42:25] <-- budlust has left IRC (Remote host closed the connection)
[17:42:42] --> budlust has joined #GemRb
[18:34:30] <-- pupnik_ has left IRC (Remote host closed the connection)
[18:55:20] <-- Maighstir_laptop has left #GemRb
[19:07:31] <tomprince> or.cz/guicallback: Mostly cleaned up.
[19:09:46] <tomprince> On the python side, I only changed those SetEvent calls where the the handler was "" or defined in the same file.
[19:14:23] <fuzzie> you checked them?
[19:14:28] --> raevol has joined #GemRb
[19:15:32] <fuzzie> and you really want to change the typedef?
[19:16:13] <fuzzie> and there's a whitespace-only change to TableMgr.h in here
[19:20:45] <fuzzie> (although if you want to make the change, any commit is as good as any other, iguess)
[19:49:40] <tomprince> Didn't check them by hand, I checked it with grep and sed and shell.
[19:50:22] <tomprince> About the typedef, It feels silly to have exactly one instance of Holder<Blah> typedefed.
[19:50:56] <fuzzie> well, typedef the rest? :)
[19:51:26] <tomprince> for file func in $(git grep SetEvent GUIScripts |sed -n "s/\(.*\):.*[\"']\(.\+\)[\"'].*/\1 \2/p"); PAGER=cat git grep -q "def ${func} \?() \?:" $file && sed -i "s/SetEvent \?(\(.*\), \?[\"']${func}[\"'])/SetEvent (\1, $func)/" $file
[19:51:38] <fuzzie> i'm not sure what non-internal non-plugin/resource stuff is Holdered
[19:51:49] <tomprince> We could do that.
[19:51:51] <fuzzie> and by 'you checked them?', i mean that the criteria there don't seem very sensible checks
[19:52:46] <fuzzie> just because it doesn't seem unlikely that a common handler might get overridden by a module
[19:52:54] <fuzzie> but i haven't thought about it and i haven't seriously looked at the code recently
[19:53:30] <fuzzie> so it is just a thought
[19:55:50] --> Maighstir_laptop has joined #GemRb
[21:05:41] <tomprince> Well, unless there is *really* wacky stuff going on, the only possibility is NextPress in char gen, and Back/CancelPress in bg1 chargen.
[21:11:07] <tomprince> In terms of events that I changed, there is overlap between GUILOAD/GUISAVE, GUIWORLD/Start, and the in game info/chargen.
[21:11:55] <tomprince> The wacky stuff would be a control that gets an event set in one place, a SetNextScript called, and then expecting to run the event in the new context.
[21:12:07] <tomprince> If we do that anywhere, then the code is broken, not my changes. :)
[21:12:42] <fuzzie> yes, that would be a bit mad
[21:14:11] <fuzzie> but you can grep for the chargen ones easily enough i thin?
[21:16:28] <tomprince> I did, but it isn't clear enough to me what is going on, so I don't know if it is safe to change or not.
[21:16:38] <fuzzie> the bg1 chargen seems clear enough
[21:17:49] <tomprince> The tricky bit is that NextPress is define in CharGenCommon, and files that include it. CharGenCommon does a SetEvent(..., "NextPress")
[21:18:11] <tomprince> If I change that to SetEvent(..., NextPress) it will change which function it points to.
[21:19:23] <fuzzie> mhm
[21:19:59] <fuzzie> i mean, it would not be safe to change
[21:20:08] <fuzzie> afaict
[21:20:42] <fuzzie> but not sure
[21:20:52] <tomprince> Yes. I have pushed a new version that does change NextPress. And also not Back/CancelPress in bg1.
[21:21:28] <fuzzie> if this is committed, i guess these should really be different functions?
[21:21:43] <tomprince> But, if my scripting-foo is right, those are the only places that are an issue.
[21:22:01] <fuzzie> and it would be nice if your sed didn't break the whitespaces, too
[21:22:41] <fuzzie> i mean, s/break/change/
[21:23:48] <fuzzie> but i mean, maybe SetEventByName or something, otherwise it's going to be kinda spectacularly unclear
[21:23:48] <tomprince> Yes, well the code has a mix of whitespace styles. I guess I could teach it not to change. But I would rather change it to be consistent.
[21:24:05] <fuzzie> well, make a commit which makes the whitespace consistent, then?
[21:24:14] <fuzzie> but your commit just makes it even more inconsistent
[21:24:53] <tomprince> Well, what do we want it to be? FunctionName(With, Paramaters, like, this) ?
[21:25:13] <fuzzie> well, that would be my preference, but i would ask lynxlynxlynx
[21:28:16] <lynxlynxlynx> vs FunctionName (With, Paramaters, like, this) ?
[21:28:33] <fuzzie> well, there are also a lot of uses of FunctioName(With,Parameters,like,this)
[21:28:45] <fuzzie> i see all of those just looking through tomprince's diff :)
[21:28:47] <lynxlynxlynx> spaces after commas, definitely
[21:29:10] <fuzzie> git makes it so easy to ignore whitespace changes, so a mass-reformat is fine with me
[21:29:52] <lynxlynxlynx> i don't have problems with the inconsistency
[21:30:08] <lynxlynxlynx> i fix them when i have legitimate reasons to change the line
[21:30:46] <fuzzie> well, i don't really care
[21:31:08] <fuzzie> but i would prefer that a patch not make the whitespace on a single line be different to the rest of the function
[21:31:54] <lynxlynxlynx> that space-before-paren thing was new to me, but it is written down as a guideline in the docs and most of the code follows it
[21:35:22] <tomprince> Well, it does mean that the code is different from the C++ side.
[21:36:27] <lynxlynxlynx> those guidelines are exclusively for the python side
[21:37:40] <tomprince> My question simply was whether we wanted different conventions for C++ and python.
[21:37:42] <fuzzie> we really want clarity over compactness on the python side, so i think we probably don't want to aim for making them as close as possible
[21:38:12] <fuzzie> but i have no opinion on this in particular, other than that i think it's really up to lynx at this point :)
[21:38:45] <lynxlynxlynx> i don't think it is an issue
[21:38:53] <tomprince> Which would you rather?
[21:39:17] <lynxlynxlynx> i would rather see time not being wasted on whitespace cosmetics
[21:40:05] <lynxlynxlynx> but if i had to choose, then it'd be the predominant style
[21:40:25] <lynxlynxlynx> which i'm pretty sure is "FunctionName (With, Paramaters, like, this)"
[21:44:02] <-- budlust has left IRC (Ping timeout: 265 seconds)
[21:46:15] <tomprince> Well, patch is updated that way.
[21:48:04] <tomprince> It is particularily easy when the patches is entirely mechanical.
[21:49:40] <fuzzie> sure, but this is hard to read:
[21:49:41] <fuzzie> SpellsPickButton.SetState(IE_GUI_BUTTON_ENABLED)
[21:49:41] <fuzzie> - SpellsPickButton.SetEvent(IE_GUI_BUTTON_ON_PRESS, "SpellsPickPress")
[21:49:44] <fuzzie> + SpellsPickButton.SetEvent (IE_GUI_BUTTON_ON_PRESS, SpellsPickPress)
[21:49:57] <fuzzie> that is my problem, re: the above
[21:50:15] <fuzzie> i don't care about it being consistent everywhere, but if it isn't locally consistent, it's just incredibly distracting
[21:53:29] <fuzzie> the patch as it is is also not touching any uses of "BackPress"?
[21:54:01] <fuzzie> + SpellsCancelButton.SetEvent (IE_GUI_BUTTON_ON_PRESS, SpellsCancelPress)
[21:54:01] <fuzzie> else:
[21:54:01] <fuzzie> SpellsCancelButton.SetEvent(IE_GUI_BUTTON_ON_PRESS, "BackPress")
[21:54:09] <fuzzie> but, must run
[21:55:04] <tomprince> Only in bg1.
[22:12:28] <-- lynxlynxlynx has left IRC (Remote host closed the connection)
[22:53:48] --> pupnik has joined #GemRb
[22:53:50] * pupnik wonders who at IBM needs troutslapping for putting FN where CTRL should be
[22:54:33] <Lightkey> not just IBM
[23:11:12] <pupnik> at least they resisted the windows key
[23:14:35] <Maighstir_laptop> Resisted the windows key? on what machine?
[23:22:09] <pupnik> older thinkpads and ibm keyboards Maighstir_laptop
[23:24:09] <Maighstir_laptop> Hmm... *checks his P3 Thinkpad*
[23:24:13] <Maighstir_laptop> Heh, you're right
[23:26:00] <Maighstir_laptop> Lenovo added it in when they got the brand though
[23:32:47] <pupnik> ysp
[23:47:17] --> budlust has joined #GemRb