GNOME Bugzilla – Bug 485919
Use Glade for UI
Last modified: 2007-11-04 02:46:15 UTC
Speaks for itself... .glade files are easier to change and mean less code to read through.
Created attachment 97102 [details] [review] Patch to add glade support and convert the mode panel This patch adds glade support and converts create_mode_panel() to using a glade file. If the file is missing gcalctool will crash - the user should be warned if the files do not exist. Will add patches for other functions.
Nice work! Thanks. I've added Calum to the cc: list. He's my HCI guy for gcalctool. Calum, for now I just want you to be aware of the work that Robert is doing here. I know that what he's done so far isn't HIG compliant (borders aren't correct), but I suggest waiting until he's done all the Glade work before it's fixed up. Robert, I've checked your patch into SVN trunk. Hopefully that will make it easier to do further increments. I noticed you called it mode_panel.glade. I was expecting everything to go into just one Glade file, and therefore it should be called something like gcalctool.glade. Is there a good reason to have multiple Glade files? Thanks.
> If the file is missing gcalctool will crash - the user should be warned > if the files do not exist. Oh, forget to add. I assume a future patch will do the right thing here? Thanks.
To make things easier to do in pieces I'm converting each function one by one to match the existing behaviour. At the end they can all be merged into the one file and Calum can tidy it up :) Yes, a future patch will check if the files exist, I'm just noting it here so it doesn't get forgotten.
> To make things easier to do in pieces I'm converting each function one by one > to match the existing behaviour. At the end they can all be merged into the one > file and Calum can tidy it up :) Okay. I'll keep applying them to SVN trunk so that they can be included in the gcalctool tarballs as they are made available. This way hopefully other people might find problems that we weren't aware of. > Yes, a future patch will check if the files exist, I'm just noting it here so > it doesn't get forgotten. Understood. Thanks.
This one is turning out to be a bigger job than I expected... Work is not on the gcalctool-glade branch.
that is _now_ on the gcalctool branch :)
Created attachment 97503 [details] [review] Patch making the majority of the gcalctool UI use glade Patch making the majority of the gcalctool UI use glade. Unfortunately this patch is huge and not easily able to be split up due to the complex relationship between gtk.c, calctool.c and functions.c
Nice. So the question on my mind is, if I apply this to SVN trunk how unstable would gcalctool be? Broken functionality? Crashs? I'll need to make a gcalctool tarball for GNOME 2.21.1 in a weeks time. Should I put this work in it? My gut feeling is yes, and just warn people that gcalctool is going through an adjustment period. Thanks! PS: I'll try it out tomorrow, but I thought I'd ask first. ;-)
The attached patch converts ~80% of the gcalctool ui to using glade (the hardest part! :). The display and behaviour should exactly match the current svn head. These changes are checked into the gcalctool-glade branch. The major parts to change are the pop-up dialogs. The change took longer than expected due to the amount of UI behaviour that is in calctool.c. Doing a bit of searching shows this seems to be a hang-over from CDE days where there was more than one UI toolkit that could be used. Is this correct? Some examples of UI behaviour in calctool.c: - The shortcut keys are used to compare if buttons are the same - The concept of a menu button is present - gtk.c should decide if buttons are multiplexed into one (e.g. the shift left button) or separate. - The mode (basic/advanced etc) is known I'd recommend that the relationship is gtk.c generates calculator events (e.g. numeric 0, shift left 5, calculate etc) and calctool.c generated display updates and error states. gtk.c can decide what buttons are present and their layout. I'm happy to make some more patches for the dialogs but I probably wont have any time to simplify calctool.c/gtk.c. I've got some Gnome Games networking problems to solve...
Rich, I don't expect any major crashes... (fingers crossed) :)
> Doing a bit of searching shows this seems to be a hang-over from > CDE days where there was more than one UI toolkit that could be used. Is this > correct? It's earlier than that calctool was originally a SunView application from 1986: http://groups.google.com/group/net.sources/msg/486bd61397719837?q=calctool+Sun+Australia+1986&hl=en&lr=&newwindow=1&rnum=2 It's come a long way since then. At one time, there were graphical "drivers" for X11, SunView, Motif, NeWS, curses, MGR and a couple others. > I'd recommend that the relationship is gtk.c generates calculator events (e.g. > numeric 0, shift left 5, calculate etc) and calctool.c generated display > updates and error states. gtk.c can decide what buttons are present and their > layout. Yeah, I understand. I doubt whether I (or anyone else) will port gcalctool to another graphical toolkit. There is no pointr in trying to keep it as generalized code. > Rich, I don't expect any major crashes... (fingers crossed) :) Okay, cool. I'll try to patch SVN trunk with it, and get something committed. When it's there, I'll ask Calum if he could HIGify it for us. Thanks.
Forgot to comment on this one: > I'm happy to make some more patches for the dialogs but I probably wont have > any time to simplify calctool.c/gtk.c. For now I'm happy if we can just get it looking and performing the way that the non-Glade one worked. Over time, it can be made cleaner.
Patch committed. Thankyou! This is really great. I suggest continue working on SVN trunk now. I'm happy to commit patches as soon as they are available. I've also added your name to the gcalctool authors. It's the least I could do for a substantial change like this. Would you be interested in becoming a gcalctool maintainer? I noticed that the calculator in Basic mode has grown slightly, so Calum, could you please do your magic and HIGify the Glade file please? I noticed the gcalctoolrc file no longer works, but I suspect nobody uses that. Calum, what are your thoughts on this. Should we just remove it, convert it or in some ways improve it so that it's automatically available.
Filed bug #488703 on a problem with strange "-/-" entries at the end of each of the Acc menu items.
Filed bug #488694 on a problem with some of the buttons not having good accessible names.
Created attachment 97625 [details] [review] Puts menu accelerators back again and fixes the top button row in basic mode being too tall
As usual Glade makes a very non-diffable file... :(
Latest patch committed. Note that I had to make a slight adjustment for also fixing bug #488830, and I used glade-2. Therefore, you are going to need to pull out the updated gcalctool.glade file again, as I know it writes some lines out with a slightly different indentation. They aren't incompatible at the Glade runtime level, just with an editor. Thanks!
Created attachment 97961 [details] [review] Patch adding keyboard shortcuts to gcalctool, needs work This patch moves the keyboard handling into gtk.c. There are a number of minor problems/interests: * The keyboard repeat rate is now different (slower) * The keys visually move now * There is no shortcut for XOR. Previously it was set to 'x' but this conflicted with the multiply key. This meant XOR never had a shortcut - we should pick a new one. * The patch has a lot of printfs for use in debugging, these need to be removed before committing * I noticed the key display values (symname) in calctool.c were set to be translatable. This was fine for the GUI but they're also used in the lexer/parser. So the calculator would break if they were translated? * There was a potential buffer overflow in do_number(): SNPRINTF(v->display+len, MAXLINE, "%c", nextchar); That should be MAXLINE-len. A quick glance shows a number of dubious sprintf/strcats around the code. And some major ones: * Taking the key codes out of calctool.c has caused me to change a lot of unrelated code. It's so obscure with all the global variables I'm not confident everything will work the same. * Arithmetic mode is completely unreliable. The data being passed to ce_parse() is the same as before but it normally returns "Malformed Expression". Try 1+1 (works) and 1+2= (doesn't work). I don't know how this code works at all so I need someone else to look into it. I really need some help with this one...
Thanks Robert! I'm going to hold off testing and committing this patch myself, until after I've created the gcalctool tarball for GNOME 2.21.1 on Sunday or Monday. > * The keys visually move now That's probably not a bad thing. They used to do that with calctool (and probably dtcalc). > * There is no shortcut for XOR. Previously it was set to 'x' but this > conflicted with the multiply key. This meant XOR never had a shortcut - we > should pick a new one. Multiplication used to be "*", but people objected so it was changed to "x". I didn't notice that that clashed with XOR. I guess the question is, "what's left?" How about "\" ? > * I noticed the key display values (symname) in calctool.c were set to be > translatable. This was fine for the GUI but they're also used in the > lexer/parser. So the calculator would break if they were translated? Yes. Nice catch. > * There was a potential buffer overflow in do_number(): > SNPRINTF(v->display+len, MAXLINE, "%c", nextchar); > That should be MAXLINE-len. A quick glance shows a number of dubious > sprintf/strcats around the code. Another nice catch. > * Taking the key codes out of calctool.c has caused me to change a lot of > unrelated code. It's so obscure with all the global variables I'm not > confident everything will work the same. Understood. We've got plenty of time to fully test this out. > * Arithmetic mode is completely unreliable. The data being passed to > ce_parse() is the same as before but it normally returns "Malformed > Expression". Try 1+1 (works) and 1+2= (doesn't work). I don't know > how this code works at all so I need someone else to look into it. Sami, could you look at this please? For now, just apply Robert's patch to a version of gcalctool checked out from SVN trunk, and see if you can work out what's happening and comment on this bug. Thanks.
Hi, > - number | bin_set | oct_set > + number Why the patch changes expression mode signaling?
Robert, you're the best person to answer this, but from a cursory look at the code, the only place that bin_set, oct_set and hex were used was in the button_cb() routine in gtk.c, to detect if that numberic digit was valid in the current base. I presume there is another way of doing this now. Robert? I just tried the patch to SVN trunk. I'm not getting simple things like "1+2=" failing. Is it possible that you didn't have the patch from Sami applied, that stabilizes arithmetic precedence? http://bugzilla.gnome.org/attachment.cgi?id=97754 From my tests, it seems to be all working nicely, and I'm seriously considering committing it before I make the GNOME 2.21.1 gcalctool tarball. Sami, are you seeing any problems?
Yes, the patch seems to break the arithmetic precedence mode. Exact reason is unknown.
I noticed a difference in this patch and the current trunk: In trunk, S, R, and X press the Sto, Rcl, and Exch buttons respectively just as if you had clicked on them with the mouse (i.e. a menu pops up). With the patch applied, I see the appropriate button's appearance change (which I think is cool btw), but the menu does not pop up and the function doesn't seem to get executed.
Just found something more significant: Clear the display and type a couple of numbers really quickly (like 12). The numbers appear in the reverse order. Thus: * 1 <pause> 2 results in 12 as expected, but * 12 results in 21 This occurs both in arithmetic and left-to-right precedence.
Sami wrote: > Yes, the patch seems to break the arithmetic precedence mode. Exact reason > is unknown Joanie wrote: > * 1 <pause> 2 results in 12 as expected, but > * 12 results in 21 Okay, these are enough reasons NOT to include this patch for GNOME 2.21.1. Thanks for testing. After Monday, we can try to continue to investigate the patch and get it working better.
Joanie wrote: > * 1 <pause> 2 results in 12 as expected, but > * 12 results in 21 Joanie, correct me if I've wrong here, but I *think* you are: 1/ Pressing down the 1 key. 2/ Pressing down the 2 key 3/ Releasing the 2 key 4/ Releasing the 1 key. You are doing it so fast you aren't realizing it. Now the problem I *think* is being caused by the difference between a "clicked" event and a "key_press_event". In the pre-Gladed gcalctool, there was: g_signal_connect(G_OBJECT(X->kframe), "key_press_event", G_CALLBACK(kframe_key_press_cb), NULL); and the kframe_key_press_cb() routine did all the handling of the key press events. In other words, it did them in order. I don't believe "clicked" events are processed until the user releases the key. In the new Glade file, I don't see an equivalent of the old style. Robert, hopefully there is enough here to help you determine what's going on.
Rich, This patch is not for committing!!! Needs lots of work... Joanie, yes I see it too. Typing 123 also quickly displays as 321. It may be the Gtk+ accelerator handling is not suitable for calculator key events. My fallback plan is to reconnect the events when loading the Glade file. Sami, the bin/oct/hex_set flags were only used in the GUI and this is enforced by making the buttons sensitive/insensitive so I didn't see the need for this. I'm hoping you can help me with the arithmetic mode as all the inputs are the same as before (as far as I can tell) but the results are different.
>Sami, the bin/oct/hex_set flags were only used in the GUI and this is enforced >by making the buttons sensitive/insensitive so I didn't see the need for this. Does this new approach handle the keyboard shortcuts? See all bug469245 use-cases for more details. >I'm hoping you can help me with the arithmetic mode as all the inputs are the >same as before (as far as I can tell) but the results are different. Try: make clean make Does this make any difference?
Robert wrote: > This patch is not for committing!!! Needs lots of work... Yup. I understand now. I won't commit it. > Joanie, yes I see it too. Typing 123 also quickly displays as 321. It may be > the Gtk+ accelerator handling is not suitable for calculator key events. My > fallback plan is to reconnect the events when loading the Glade file. Checkout whether adding a key_press_event handler for the equivalent of X->kframe fixes this. If it does, you might also have to add back the bin_set, oct_set and hex flags.
Created attachment 98224 [details] [review] Patch with working (hopefully!) keyboard handling This patch supports keyboard handling as is currently performed on the svn head version. I have removed all concept of key bindings from calctool.c but unfortunately I wasn't able to move the accelerators into the .glade as: - The behaviour was different (i.e. the 123 -> 321 problem) - GTK+ doesn't seem to have an ability to pull the accelerators off the widgets and reconnect them. I tried lots of different methods to get the accelerators but none of them worked (or required using (semi-)private fields in the objects). This should be raised with the GTK+ team one day... I also noticed a bug which is also present in the head version: 1. Got to scientific mode 2. Enable hexadecimal 3. Enter ABC 4. Change to decimal mode 5. "no sane value to convert" error is displayed. If you change back to hex and then back to decimal it seems to work OK. Or if you press enter before changing to decimal.
Sami, the arithmetic problems seemed to fix themselves after the make clean, thanks!
Created attachment 98225 [details] [review] Rebuilt after checkout Same comments as before, just hadn't done the svn update before hand
I've gone ahead and committed the last patch as I'm having a sick day off from work and making other progress... Hope this is OK?
Created attachment 98229 [details] [review] Moves ASCII, accuracy, function and constant dialogs into gcalctool.glade
> I've gone ahead and committed the last patch as I'm having a sick day > off from work and making other progress... Hope this is OK? No problem at all. > Created an attachment (id=98229) [edit] > Moves ASCII, accuracy, function and constant dialogs into gcalctool.glade Excellent. Looks like you've committed this one too. > I also noticed a bug which is also present in the head version: > 1. Got to scientific mode > 2. Enable hexadecimal > 3. Enter ABC > 4. Change to decimal mode > 5. "no sane value to convert" error is displayed. > > If you change back to hex and then back to decimal it seems to work OK. Or if > you press enter before changing to decimal. I think this is bug #490701, which was just fixed by Sami 3 days ago. Thanks!
I think the Accuracy button is somewhat broken. (Noticed this on the weekend rather than as a result of the past couple of patches) Try this: 1. Get into the Scientific calculator 2. Click on Acc and choose other... 3. Pick a value you haven't picked yet, like 10 4. Go back and choose a value in the menu, like 3. Expected results: That menu item would be selected. Actual results: The "other" dialog reappears asking you for the precision. 5. Humor the dialog and type 3. (The precision changes) 6. Go back and choose another value in the menu, like 4. Expected results: That menu item would be selected and the precision would change. Actual results: That menu item is selected but the precision does not change. At this point, choosing any precision via the menu (rather than through other) seems to fail.
A small buglet: The window title "Edit Constants..." or "Edit Functions..." is not being set in create_cfframe() in gtk.c. Just look for the title variable. It's being set but not used. Old code was: const gchar *title; ... title = (mtype == M_CON) ? _("Edit Constants") : _("Edit Functions"); ... if (dialog == NULL) { dialog = gtk_dialog_new_with_buttons(title, NULL, GTK_DIALOG_DESTROY_WITH_PARENT, GTK_STOCK_HELP, GTK_RESPONSE_HELP, GTK_STOCK_CANCEL, GTK_RESPONSE_REJECT, GTK_STOCK_OK, GTK_RESPONSE_ACCEPT, NULL);
Rich, that's being fixed in the next patch... I've split the function and constant dialogs into two separate GUI elements (the labels in the function dialog don't seem to make sense and sharing the code makes the code less readable for little gain). Joan, The accuracy button confuses me too. I'm hoping things will become clearer once the Glade changes have been finished.
Created attachment 98317 [details] [review] Move all popup menus into gcalctool.glade and split constant and function dialogs into seperate UI elements.
Created attachment 98318 [details] [review] Simplify menu code
And that's all the major changes I wanted to do done... I've got rid of the ping-ponging when doing menu buttons and v->pending with that last patch. I think that was the most complex part of UI code there. I hope I haven't unfairly imposed my architecture on the code, questions/comments/patch reversions (though I don't recommend going back!) welcome. I can confirm the accuracy menu doesn't work at the moment. We need to work out what broke that... Please check if there have been any reversions. The only other changes I want to make sometime in the future is: - Split gtk.c into ui.c, ui-display.c, ui-etc.c - Add header files with full comments for each function - Reduce/rationalize the number of variables in in calcVars and Xobject but they're for another rainy day.
This is great! Nice work. > The only other changes I want to make sometime in the future is: > - Split gtk.c into ui.c, ui-display.c, ui-etc.c > - Add header files with full comments for each function > - Reduce/rationalize the number of variables in in calcVars and Xobject > but they're for another rainy day. These are all fine with me. Code documentation is sorely needed. B.t.w. it probably wasn't obvious, but the two main reasons why there are calcVars and Xobjects (as opposed to oodles of single variable all over the place) were: 1/ They would be contiguous in memory and therefore reduce swapping on small memory systems (this was from a time when 16Mb systems were very popular). 2/ You could just say "print *v" or "print *X" and you got everything. Again, from a time when GUI based debuggers weren't ubiquitous. I'll try to give it more linting soon, once we are sure it's mostly working like the older version. I suspect we have several routines and variable that are no longer needed. Thankyou.
I've just given it a cursory testing, and the only thing I could find wrong is the Accuracy anomaly that Joanie found. There's probably others lurking but overall, it seems to be in pretty job shape. Most impressive Robert. Once the Accuracy bug has been put to bed, I suggest that you can close this bug, and we'll open up other ones for any new problems that are found.
Created attachment 98436 [details] [review] Fixes GUI accuracy button problems
The last patch fixes the accuracy button not working. Closing this bug (yay!).
(In reply to comment #47) > The last patch fixes the accuracy button not working. Closing this bug (yay!). Congrats!!! :-) That was clearly a Herculean effort.... So it pains me to point out the following: It fixes it if you use the mouse, if you use the keyboard shortcut assigned to it (A), it: 1. Sets the accuracy to the first item in the menu (that fails to pop up) 2. Fails to update the radio buttons in that menu to reflect the change The menu not popping up seems to be another instance of what I mentioned earlier: (In reply to comment #25) > I noticed a difference in this patch and the current trunk: > > In trunk, S, R, and X press the Sto, Rcl, and Exch buttons respectively just as > if you had clicked on them with the mouse (i.e. a menu pops up). With the > patch applied, I see the appropriate button's appearance change (which I think > is cool btw), but the menu does not pop up and the function doesn't seem to get > executed. Now it seems that pressing any of the keys with associated pop-ups (including Accuracy) do cause the function to get executed. However I think that what is expected (and desired) is for the menu to appear.
Missed that one, fixed now.
Created attachment 98439 [details] [review] Makes shortcut keys for menu buttons cause popups to display the final final patch...
(In reply to comment #50) > the final final patch... Famous "last words." ;-) 'fraid I found something else. It's not a result of the final final patch, and it's something I should have noticed last night but didn't (Sorry!): The pop-up menus which store values (Con, Fun, Sto, Rcl, Exch) have items in the form: C_0: 0.621 where the underscore is actually a character in between 'C' and '0'. Before the '0' was underlined. Aesthetics aside, you can no longer use the (non)underlined digit to quickly select an item in these menus.
Created attachment 98469 [details] [review] Patch to fix the last reported problem. Joanie, I think the attached patch fixes the last problem you are seeing. Could you give it a try please? Thanks.
Indeed it does. Thanks.
Cheers Rich!
Also noticed the display was now user editable (allowed arbitrary input). Fixed on head by changing editable property on GtkTextView to FALSE
> Also noticed the display was now user editable (allowed arbitrary input). > Fixed on head by changing editable property on GtkTextView to FALSE Actually, can you back out that "fix". This is a new feature. See bug #326938 for more details. It looks like the version in SVN trunk now breaks this in other ways: See the last patch for bug #326938 http://bugzilla.gnome.org/attachment.cgi?id=96904 Can we get that functionality reinstated please? Thanks.