**** BEGIN LOGGING AT Wed Jul 28 02:59:56 2010 Jul 28 16:06:36 kvalo: Have you tested the v3 patch from Zhenhua? Jul 28 16:08:38 denkenz: no. and I can't do it before friday Jul 28 16:29:52 sok Jul 28 18:17:00 balrog-k1n: So question, why do we bother messing with the main menu property, making it null, etc? Jul 28 18:17:57 balrog-k1n: Aren't we getting the same thing by having RegisterAgent returning an error? Jul 28 18:18:05 Or SelectItem rather Jul 28 18:33:00 denkenz: so that the menu is not displayed to the user Jul 28 18:33:31 Why not? Jul 28 18:33:45 I think that is actually more confusing Jul 28 18:34:03 because it's wrong to display it just to pop up an error message when the user chooses anything Jul 28 18:34:21 the menu is effectively unavailable during the session, Jul 28 18:34:27 so it should not be available through dbus either Jul 28 18:35:03 I disagree, consider a typical app Jul 28 18:35:33 You get a menu, then lets say one of the menu items triggers a display text Jul 28 18:35:51 You pop that up, user cancels and hides the display text window Jul 28 18:35:57 Now he's staring at a blank screen Jul 28 18:36:11 Some time later the main menu gets re-populated Jul 28 18:36:20 To me that sucks Jul 28 18:36:25 no, when he cancels the text window ofono goes back to main menu immediately Jul 28 18:36:36 it's the implementation of the screen management part of the spec Jul 28 18:36:39 which still takes time Jul 28 18:36:46 if you don't do that you force the ui writer to read all of it and implement it Jul 28 18:37:39 my opinion is still that the main menu should be an agent request like the submenus, text etc, Jul 28 18:38:25 I'm mostly fine with how this is working out Jul 28 18:38:39 But the main menu changing seems wrong Jul 28 18:38:50 surely you should not have access to the main menu and a submenu at the same time Jul 28 18:39:06 it would be wrong to leave it populated Jul 28 18:39:12 You won't, because you get an error Jul 28 18:39:32 only after you select, which would just suck Jul 28 18:39:49 Not really Jul 28 18:39:59 No sane UI is going to let the user into the main menu during an operation anyway Jul 28 18:40:39 yeah, but the api is designed so that many UIs can be running Jul 28 18:40:58 i understand that this was the reason of the last re-design too Jul 28 18:41:05 The problem here is that messing with the menu is not a good way Jul 28 18:41:19 I'd rather accomplish this through a State property Jul 28 18:41:45 i don't see the problem with changing the menu Jul 28 18:41:48 Messing with the Menu you make other situations ambiguous, like if the SIM really sends us a different menu Jul 28 18:42:20 what could happen if the sim sends a different menu? Jul 28 18:42:53 That still gets treated as a property changed Jul 28 18:43:00 exactly Jul 28 18:43:09 So it could send us an empty menu too, or send us a setup menu during a session Jul 28 18:43:24 Again, IMO it is a bad idea Jul 28 18:43:41 the code handles that Jul 28 18:43:45 whenever the available options change, you get a PropertyChanged Jul 28 18:44:53 Fair enough, but I still don't see what its gaining the application writer Jul 28 18:45:06 All you get are screen flickers Jul 28 18:45:11 Completely pointless Jul 28 18:45:35 i don't see how it'll flicker more than if the UI implements the hiding and showing of main menu Jul 28 18:45:45 the menu needs to disappear as soon as it's unusable Jul 28 18:45:51 which is the moment a session starts Jul 28 18:45:56 Because DBus has latency Jul 28 18:46:00 And fairly big one Jul 28 18:46:09 and it should appear no sooner than it becomes available again Jul 28 18:46:25 That's not how e.g. iPhone UI works Jul 28 18:46:28 Play with it Jul 28 18:46:54 don't have any iphone near, what does it do? Jul 28 18:47:04 does it let you select from main menu even when an app is running? Jul 28 18:47:11 So it works roughly like this: Jul 28 18:47:18 You go into Sim Apps -> see Main Menu Jul 28 18:47:33 Select something, the screen transitions from Main Menu to say Display Text Jul 28 18:47:54 Or even Select Item for sake of argument Jul 28 18:48:03 ok Jul 28 18:48:11 Select something on Select Item, the Main Menu appears Jul 28 18:48:18 Then screen transitions to Display Text Jul 28 18:48:28 User exits, back to Main Menu Jul 28 18:48:42 All items on the menu are always visible Jul 28 18:48:51 And you get to see that splash screen many times Jul 28 18:49:10 that seems wrong, my early version of the python client worked like this, Jul 28 18:49:19 but that's exactly what a ui needs to avoid imho Jul 28 18:49:28 and that is pointless flickering Jul 28 18:49:41 that's the point, the iPhone doesn't flicker Jul 28 18:49:45 It has transitions Jul 28 18:50:03 However, if you transition from an empty menu to something, that's pretty silly Jul 28 18:50:14 ok, but the dbus interface is completely orthogonal to how the transitions happen Jul 28 18:50:25 Actually it isn't Jul 28 18:50:30 Lets see a simple case Jul 28 18:50:40 Populate menu into mainwidget Jul 28 18:50:44 mainwidget.show() Jul 28 18:50:50 Select -> DisplayText Jul 28 18:50:58 -> signal Main Menu changed arrives Jul 28 18:50:59 and now that i think of it showing the main menu in between the command executing (e.g between a select item and display text) is agains the spec, however it's easy to do it wrong Jul 28 18:51:07 User hides Display Text -> blank screen Jul 28 18:51:13 20 ms later Main Menu changed arrives Jul 28 18:51:15 Flicker Jul 28 18:51:48 it should not appear immediately because it's not available immediately Jul 28 18:52:00 Then what should appear? Jul 28 18:52:01 the menu should only appear once the session ended and even the spec says so Jul 28 18:52:26 when you can't do anything, nothing should appear, or the text should still be fading out during the next 200ms Jul 28 18:52:56 imagine the following situation: Jul 28 18:53:01 I have never seen a phone do it that way Jul 28 18:53:04 All dump you to the main menu Jul 28 18:53:22 user hides Display Text, Jul 28 18:53:25 Cite some paragraphs from the spec too :) Jul 28 18:53:46 you show the menu for 50ms and the following Display Text arrives and you hide the menu Jul 28 18:53:50 that's flickering Jul 28 18:54:20 That too Jul 28 18:54:34 But it gives context to the user where he is Jul 28 18:54:40 As opposed to a blank screen Jul 28 18:54:42 but it's just wrong Jul 28 18:54:50 and i wouldn't want to use such an ui Jul 28 18:55:44 Fair enough, I wanted the entire thing in Homescreen anyways Jul 28 18:55:50 This makes things a bit simpler Jul 28 18:56:26 However, even if the UI were to be done your way, we still don't need to mess with the menu Jul 28 18:57:20 there's no good place that i can see in the spec that tells you when the menu should be displayed, Jul 28 18:57:46 but i've been reading 102.223 6.9 a couple of times and the only way it could work is if the menu is only available outside of a session Jul 28 18:58:06 If no proactive command is pending (status response of '90 00' following the terminal Response), then the session Jul 28 18:58:10 releases the display back into terminal control. If this session was terminated in a backwards move, and the session was Jul 28 18:58:13 initiated from an Envelope command containing a Menu Selection, it is recommended that the display returns to the Jul 28 18:58:17 Setup Menu. Jul 28 18:59:23 it'll also avoid confusion with multiple clients trying to use the dbus api Jul 28 18:59:39 which is not our main use case, but i think you and holtmann wanted it to be handled Jul 28 18:59:44 See, for me the above paragraph is easily accomplished using Release() Jul 28 19:00:17 yes, but then the UI writer needs to implement it Jul 28 19:00:34 instead of we being nice and forcing the UI to be correct with regards to the specs Jul 28 19:00:36 For the multiple clients using STK menus seems wrong. At least in the SelectItem case. Jul 28 19:00:49 Just return an error if we already have a user. Jul 28 19:01:03 And for the default agent, just make it simple and only one can exist at a time. Jul 28 19:01:20 I agree with holtmann here, I honestly don't see multiple clients being a proper usecase Jul 28 19:01:27 We have homescreen and simapp, that's it Jul 28 19:01:43 yeah, i'm not questioning only one session agent and one default agent possible Jul 28 19:01:53 i totally agree with holtmann and the code also agrees Jul 28 19:02:13 You're arguing for multiple simapps co-existing Jul 28 19:02:16 Which I also don't see Jul 28 19:02:33 Just return a proper error on SelectItem() and RegisterAgent() if some apps try to co-exist. Jul 28 19:02:37 denkenz: no, i'm not, but i would like this case to be handled correcrtly if it happens Jul 28 19:02:54 holtmann: sure Jul 28 19:03:01 So it will, the app can show the main menu but fail when it tries to do a SelectItem Jul 28 19:03:11 The right handling is to error out. If one simapp hogs the interface, then that app needs to be fixed to release it. Jul 28 19:03:15 why should it even show the main menu, that's wrong Jul 28 19:03:15 The user should be clued in by some other UI on his screen Jul 28 19:03:47 I agree, showing the main menu and then failing when selecting an item is just fine with me. Jul 28 19:03:59 it's pointless to show the main menu if it's not available Jul 28 19:04:02 That will never ever happen on a real devices. The UI design will forbid this. Jul 28 19:04:24 And in case that happens anyway, the UI designers have to get back to the drawing board and need to be hit with clue bat. Jul 28 19:05:03 balrog-k1n: I see where you're coming from, I do, but messing with MainMenu is not the right way Jul 28 19:05:15 If we really need this, then using State is better Jul 28 19:05:21 i also wouldn't like to see a UI where the main menu pop ups all the time for super short amounts of time, bleeding through your app, and confusing the user Jul 28 19:05:29 balrog-k1n: This is a problem of the application and more a problem of the UI design. They mess up, they fix their design. Jul 28 19:05:57 balrog-k1n: If this happens, then the UI design is broken. Jul 28 19:05:59 ok, but they don't even need to think about it if we do it correctly Jul 28 19:06:31 There should be one simapp and one homescreen. That is what they should target. If they do something else, then so be it. Jul 28 19:06:39 Their problem and they have to deal with it. Jul 28 19:06:52 If this means that they show the main menu and then selecting an item fails, that is fine. Jul 28 19:07:15 Just don't worry too much here. Jul 28 19:07:23 holtmann: it's also about a single UI app showing the main menu in the middle of you walking some deep submenu for short periods of time Jul 28 19:07:38 there's no point making the main menu available at that time Jul 28 19:07:48 Then that is up to the UI Jul 28 19:07:56 However, both should be possible Jul 28 19:08:14 If someone wants to do an iPhone like UI, cool, if someone wants to be more fancy, great Jul 28 19:08:20 only the behaviours that are allowed by the spec should be possible Jul 28 19:08:34 And the spec does not preclude anything :) Jul 28 19:09:32 All the spec says is '... it is recommended...' Jul 28 19:09:46 Nothing about must Jul 28 19:09:47 by my reading it does.. but we're looking at a pretty unclear spec so you have to try to understand what the authors meant Jul 28 19:11:08 And honestly I never had the same impression when I read the spec, but I was less thorough Jul 28 19:13:59 i don't know, i think the menu items list should only list things that SelectItem() can accept Jul 28 19:14:19 if it can't accept any of the choices then let's not pretend there is any choice Jul 28 19:15:43 Again, I think your concern is fairly artificial Jul 28 19:15:51 There's never going to be a case like this Jul 28 19:16:33 there will always be a case like this in the 50ms between a SelectItem() and the following RequestSelection() for example Jul 28 19:17:00 So the UI has two choices, they show a different splash screen or they recognize the InProgress error and ignore it Jul 28 19:17:16 or after the user selects Go Back and before the previous menu pops up Jul 28 19:17:56 Or they track the lifetime of the agent and don't allow selections by graying the menu out Jul 28 19:18:07 All of this takes about 3 lines of code Jul 28 19:18:23 yeah, they can do that too Jul 28 19:20:09 So I'm thinking I will accept the first 5 patches, do some cleanup and have you re-submit display text Jul 28 19:21:45 Anyway, don't worry, I actually made up my mind on this when your first series was submitted ;) Jul 28 19:22:01 Just wanted to see if anyone would outvote me :) Jul 28 19:22:19 it's not a big change in ofono either Jul 28 19:22:57 i can just make the main menu always available and resend the last 3 patches assuming i have no other choice :) Jul 28 19:23:47 because there are unused functions in the first patch, the whole thing doesn't compile without the last patch with warnings treated as errors Jul 28 19:24:53 Eek Jul 28 19:25:12 Every patch should compile on its own ideally :( Jul 28 19:25:46 yeah, but with warnings as errors you can only add utility functions logically if they are exported in a header Jul 28 19:25:51 if they're local you get errors Jul 28 19:27:07 Yeah fair enough, sometimes there is no good way Jul 28 19:27:18 So lets do this then, quick review: Jul 28 19:27:36 +struct stk_app_agent *app_agent_create(const char *path, const char *sender, Jul 28 19:27:37 + ofono_bool_t is_default, Jul 28 19:27:37 + void *user_data, GDestroyNotify notify); Jul 28 19:27:45 Please change it to stk_app_agent_new() Jul 28 19:27:56 and also remove the user_data and GDestroyNotify Jul 28 19:28:11 Add those arguments to each and every function call, like request_selection() Jul 28 19:28:24 +void app_agent_remove(struct stk_app_agent *agent); Jul 28 19:28:29 Rename this to stk_app_agent_free() Jul 28 19:28:51 but it's notification for when the agent is destroyed, stk.c needs it Jul 28 19:29:24 Then I prefer an explicit callback setter Jul 28 19:29:30 e.g. set_disconnect_watch() Jul 28 19:29:47 Actually, name everything into stk_agent Jul 28 19:29:53 how about set_destroy_watch? Jul 28 19:29:53 not stk_app_agent Jul 28 19:30:05 That's fine Jul 28 19:30:29 +void app_agent_request_selection(struct stk_app_agent *agent, Jul 28 19:30:29 + const struct stk_menu *menu, Jul 28 19:30:29 + app_agent_selection_cb cb, int timeout); Jul 28 19:30:29 + Jul 28 19:30:42 I don't get which data gets passed in app_agent_selection_cb.. Jul 28 19:30:55 So you might want to add ofono_destroy_notify and user_data arguments Jul 28 19:31:01 all callback get passed the same data Jul 28 19:31:07 callbacks* Jul 28 19:31:26 Which is? Same thing you passed to _create? Jul 28 19:31:35 yeah Jul 28 19:31:41 Yeah, bzzt ;) Jul 28 19:31:54 Don't do that Jul 28 19:32:16 It becomes very confusing, and sometimes you want different data Jul 28 19:32:50 +void append_menu_items_variant(DBusMessageIter *iter, Jul 28 19:32:51 + const struct stk_menu *menu); Jul 28 19:33:00 i guess it's confusing if you're used to glib perhaps Jul 28 19:33:02 Can we have this function take stk_menu_items * instead? Jul 28 19:33:17 or stk_menu_item * rather Jul 28 19:33:23 yes Jul 28 19:33:30 i'll change it Jul 28 19:33:49 Great, I think that'll make more sense Jul 28 19:34:27 Patch 3 is basically without any issues from me Jul 28 19:36:41 the reason to name it app_agent i think was that bluez now has more agents than one so the names needed to be changed to be unambiguous Jul 28 19:37:07 Yeah, but stk_agent covers that, no? Jul 28 19:37:35 maybe, and since the file is named stkagent.c already.. Jul 28 19:37:49 I just don't like the asymmetry of stk_app_agent and app_agent_foo Jul 28 19:37:56 so lets do stk_agent for everything Jul 28 19:39:02 + if (dbus_message_get_args(stk->pending, NULL, Jul 28 19:39:02 + DBUS_TYPE_BYTE, &selection, Jul 28 19:39:03 + DBUS_TYPE_OBJECT_PATH, &agent_path, Jul 28 19:39:03 + DBUS_TYPE_INVALID) == FALSE) { Jul 28 19:39:03 + reply = __ofono_error_failed(stk->pending); Jul 28 19:39:04 + Jul 28 19:39:04 + goto out; Jul 28 19:39:04 + } Jul 28 19:39:15 So this part is fine, but this should seriously be done in SelectItem Jul 28 19:39:19 Not the envelope callback Jul 28 19:39:27 And return a proper error here Jul 28 19:40:08 it's already done in SelectItem, so the error here would indicate that something's wrong with ofono not the parameters :) Jul 28 19:40:35 we basically read the parameters from the message two times to avoid duplicating the data Jul 28 19:41:19 Checking it again is pretty pointless, but OK Jul 28 19:41:35 yeah, maybe there's no point checking the return value Jul 28 19:42:17 The pedantic in me thinks that agent_path should be double checked against oFono's own path validator function Jul 28 19:42:32 In case we're running on D-Bus with internal integrity checks disabled Jul 28 19:43:07 __ofono_dbus_valid_object_path Jul 28 19:43:12 However, maybe I'm just paranoid Jul 28 19:43:34 i can add that, didn't know about it Jul 28 19:44:07 In theory D-Bus should check that it is a valid object path Jul 28 19:44:13 But I've had issues with that before Jul 28 19:44:42 Can we get rid of stk->in_session completely? Jul 28 19:45:31 the presence of the session_agent should be enough Jul 28 19:45:49 yes, i think we can Jul 28 19:46:13 assuming the menu is visible even when it shouldn't be ;) Jul 28 19:46:26 Yeah yeah ;) Jul 28 19:48:01 Remember, this is all just a first attempt Jul 28 19:48:12 We'll change it in oFono 1.0, then 2.0, then 3.0, etc ;) Jul 28 19:48:51 true, but we could do it correctly from the beginning and save time :) Jul 28 19:49:35 I'd like to borrow your time machine or the prescient being you have stashed away in your pool Jul 28 19:51:31 For the last patch, I'm having trouble seeing the need for session_ended Jul 28 19:52:43 how will we know if we should kick out the session agent after a DisplayText without it? Jul 28 19:53:19 The spec allows us to release screen resources after session has ended Jul 28 19:53:32 Can't we simply use the old logic? Jul 28 19:53:48 yeah, but the card may send us a DisplayText and and the session immediately, Jul 28 19:53:53 the use will have no chance to see it Jul 28 19:54:04 user* Jul 28 19:55:01 Ok fair enough Jul 28 19:55:24 I think I'm fine with the rest of it Jul 28 19:55:38 I might have to go in and look whether we're returning errors properly everywhere Jul 28 19:55:45 As we might need to add a few Jul 28 19:56:03 we're not returning not_authorized for example Jul 28 19:56:18 Yeah, so we should add that to dbus.c/ofono.h and make use of it Jul 28 19:56:21 because src/ofono.h doesn't have it yet Jul 28 19:56:52 But that is stuff I can do too, so just make the above mods Jul 28 19:56:59 And I should get it upstreamed finally Jul 28 19:56:59 ok Jul 28 20:07:28 so i don't see any utility for the destroy callback for every agent command, there's already the callback that receives the reply Jul 28 20:07:46 the atom driver calls also have no destroy callback, just a reply callback Jul 28 20:08:18 for example in stk_agent_request_selection() Jul 28 20:08:36 the destroy callback would always happen together with the reply callback Jul 28 20:26:10 If you always guarantee the reply gets called, then destroy can be dropped Jul 28 20:29:33 yeah, it will always be called Jul 28 20:31:41 Then drop it, its fine Jul 28 20:32:06 I just didn't like the implicit userdata part Jul 28 21:27:48 denkenz: i sent a new version, tested that it still works Jul 28 21:28:07 Ok, cool Jul 28 21:28:31 now i just need to teach the clients to show the hourglass / the spinning thing even when menu is available but we're waiting for the sim to do something, instead of popping up the main menu all the time Jul 28 21:30:54 the commits history for the ui is a complete mess Jul 28 21:41:44 Heh Jul 28 21:46:09 balrog-k1n: Gah, user_cb, is void * not good enough? Jul 28 21:47:03 denkenz: in practice it should work, but in theory you can't cast function pointers to non-function pointers Jul 28 21:47:35 glib has some kind of generic function pointer type, but it's in gobject Jul 28 21:47:41 GCallback Jul 28 21:47:51 Then using a union would be better Jul 28 21:48:22 in this case we just need a function pointer, so no need to use union Jul 28 21:49:08 And btw, drivers/atmodem/* does callback casting all the time Jul 28 21:49:22 That is the whole premise behind cb_data Jul 28 21:49:50 oh right Jul 28 21:50:13 maybe there should be a ofono type for function pointers and two macros Jul 28 21:50:46 Sure, feel free to propose something Jul 28 21:50:58 In the meantime void * would be preferable Jul 28 21:51:38 +void stk_agent_remove(struct stk_agent *agent); Jul 28 21:51:43 stk_agent_free please ;) Jul 28 21:54:15 so void * is prettier because the cast is implicit but it's incorrect :) Jul 28 21:54:55 casts are a standard element in c Jul 28 21:56:24 Honestly the proper way to do this is to pass something like cb_data into stk_agent_request_start Jul 28 21:56:44 That way the callback can cast the data and the callback into the proper type Jul 28 21:57:26 Or alternatively you can define a struct / union ;) Jul 28 21:57:51 However, implicit casts to / from void are no less evil than the explicit casting you're doing Jul 28 21:58:06 But look heck of a lot prettier ;) Jul 28 21:58:22 so i see union as a more correct way any thing else will also be casting, so it's all the same Jul 28 21:58:30 And the compiler would warn you if you actually screw up. Jul 28 21:58:34 whether you wrap it in structs or not Jul 28 21:58:41 After a cast the compiler can't help you anymore. Jul 28 21:58:46 Keep that in mind. Jul 28 22:03:03 Both are acceptable Jul 28 22:03:13 We use the union in gprs_context Jul 28 22:03:23 And we use void for cb_data everywhere Jul 28 22:04:16 Whether one is more correct than the other is up for debate Jul 28 22:04:28 Not like the union will save you if you pick the wrong union member ;) Jul 28 22:04:50 yeah, there are many ways to screw up Jul 28 22:05:30 Hence readability and standards is the only real weapon Jul 28 22:05:41 well, effective weapon anyway Jul 28 22:06:30 yes, but saying we won't use explicit casts is much like saying we won't use malloc because it leads to memory leaks Jul 28 22:06:38 nokia acutally did that when they wrote gpsd Jul 28 22:06:59 We use casts Jul 28 22:07:06 But we don't allow gratuitous use of casts Jul 28 22:07:43 Especially if there's a better way and the compiler helps Jul 28 22:08:03 casting from one function pointer to another is completely safe if you cast back later, exactly same as void * Jul 28 22:08:21 and same as using a union Jul 28 22:08:28 Nobody is denying that Jul 28 22:08:49 But a) you're introducing a pointless typedef in public header Jul 28 22:09:00 b) You're introducing tons of unneeded casts throughout the code Jul 28 22:09:22 hey, you asked for the typedefs Jul 28 22:09:28 and there are exactly two casts Jul 28 22:09:48 one *to* the storage type and one from Jul 28 22:10:06 Seriously, this one is pointless: Jul 28 22:10:07 +typedef void (*stk_agent_generic_cb)(enum stk_agent_result result, Jul 28 22:10:07 + void *user_data); Jul 28 22:10:53 You're introducing extra complexity for zero gain Jul 28 22:11:17 20:41 < balrog-k1n> denkenz: if we move all the request handling to stkagent.c w Jul 28 22:11:17 e will need a struct for each request's parameters and one for each response (or a function typedef) Jul 28 22:11:20 20:41 < denkenz> Function typedef is fine, e.g. see bluez/src/agent.h Jul 28 22:11:41 There's nothing conflicting here Jul 28 22:11:52 so how is the type pointless? Jul 28 22:11:53 The function typedefs for the callback types are fine Jul 28 22:12:02 The generic callback is pointless Jul 28 22:12:10 stk_agent_generic_cb is the callback type for DisplayText, Jul 28 22:12:13 you won't avoid it Jul 28 22:12:45 Then name it like that ;) Jul 28 22:12:56 And stick it into the display text patch Jul 28 22:13:10 i named it generic because other requests whose return type is "void" will use the same callback type Jul 28 22:14:12 Actually in this case I prefer using callback typedef for each Jul 28 22:14:26 And again, you're abusing things by re-using this for some casting non-sense Jul 28 22:15:00 that's exactly the same amount of non-sense as using void *, you even admitted it Jul 28 22:15:38 or as using union, none of them looks any better Jul 28 22:15:41 Dude, I admitted its equivalent Jul 28 22:15:49 Not that one is not _preferred_ Jul 28 22:16:04 i think it's even safer to have an explicit cast Jul 28 22:17:10 it's not a beauty contest :) Jul 28 22:17:33 Request denied ;) Jul 28 22:17:38 balrog-k1n: My general rule is to avoid explicit casts whenever possible. There are only a few rare occasions where we needed them. Jul 28 22:17:59 The problem is that once you cast the compiler will obey and not question you anymore. Jul 28 22:18:20 At some point in the future you might change code bits and then it keeps crashing and we are hunting that bug for weeks. Jul 28 22:18:26 holtmann: but you can only replace them with constructs that have exactly the same property Jul 28 22:18:42 you can choose to avoid some, sure, like you can avoid mallocs Jul 28 22:18:58 I choose to avoid 99% of them. Jul 28 22:19:27 And btw. the obexd agent code had one of these casts where 50% of the time it was fine by luck of the memory layout, the other 50% it crashed. Jul 28 22:19:41 so i'm looking at bluez/src/agent.c and it uses void *cb Jul 28 22:19:44 It costs us a long time to figure that out. And the reason why we never saw it from the compiler logs was a cast. Jul 28 22:19:51 this forces the compiler to obey Jul 28 22:21:29 It is not the perfect solution. For sure. But my favorite is always against any cast. Jul 28 22:24:05 i don't think there's any better solution actually, you can replace it with a union Jul 28 22:24:17 but all your arguments apply againt union the same way Jul 28 22:25:06 All ways are equally bad in the end, just one way is cleaner to read and more consistent with the rest of the project Jul 28 22:25:12 Why are we still arguing about this? Jul 28 22:25:52 and incorrect because it casts a function pointer to non-function-pointer Jul 28 22:26:52 And since we don't care about any compiler except gcc, this is a problem why? Jul 28 22:27:30 Considering just how many gcc-isms we use, that is a lost cause argument Jul 28 22:27:57 well yeah, it just triggers an alarm Jul 28 22:28:08 in someone who reads it Jul 28 22:29:04 i don't care either way Jul 29 02:01:49 holtmann: are u still there? 5 patches including btio.[ch] are for oFono. shall i tell Johan and Luiz to review them against latest oFono tree? Jul 29 02:37:06 Yes please. Let them know about the btio.[ch] patches. **** ENDING LOGGING AT Thu Jul 29 02:59:57 2010