GNOME Bugzilla – Bug 109254
Dave's Megapatch
Last modified: 2004-12-22 21:47:04 UTC
Sorry I didn't send this last nite (probably morning for you) I just needed to go to sleep.... what this patch does: 1. Unify file->rename, file->delete, these are in the file menu as they act upon the actual file not the content of the file. This is where these items have always been in windows and on the mac, and even in nautilus until recently (nautilus is a little messed up here, but i won't really go into it here). 2. Add cut, copy, paste and select all to the edit menu. select all works in the tree too. 3. Add a topics popup menu with rename and delete in it. 4. Use an hpane insted of an hbox in the bme. 5. Use delete key to delete topics (had to do this in a keypress event, setting the default binding of the "delete" menu item to delete screws up text editting. 6. Sort the topics in the properties/new bookmark window case insensitively by name. 7. Click anywhere on a topic in the properties/new bookmark window to add or remove (I still need to get this to work by clicking space or enter)
Created attachment 15212 [details] [review] please read it closely, i'm probably doing dumb stuff :)
-static void cmd_remove_bookmarks (EggAction *action, +static void cmd_delete (EggAction *action, EphyBookmarksEditor *editor); static void cmd_bookmark_properties (EggAction *action, EphyBookmarksEditor *editor); static void cmd_add_topic (EggAction *action, EphyBookmarksEditor *editor); -static void cmd_remove_topic (EggAction *action, +static void cmd_rename (EggAction *action, EphyBookmarksEditor *editor); -static void cmd_rename_bookmark (EggAction *action, +static void cmd_close (EggAction *action, EphyBookmarksEditor *editor); -static void cmd_rename_topic (EggAction *action, +static void cmd_cut (EggAction *action, EphyBookmarksEditor *editor); -static void cmd_close (EggAction *action, +static void cmd_copy (EggAction *action, + EphyBookmarksEditor *editor); +static void cmd_paste (EggAction *action, + EphyBookmarksEditor *editor); +static void cmd_select_all (EggAction *action, EphyBookmarksEditor *editor); Here you borked all align ;) + } + + else if (ephy_node_view_has_focus (editor->priv->key_view)) No new line here + else if (ephy_node_view_has_focus (editor->priv->key_view)) + { + GList *selection; + + selection = ephy_node_view_get_selection (editor- >priv->key_view); + + if (selection) + { + EphyNode *node = EPHY_NODE (selection->data); + ephy_bookmarks_remove_keyword (editor->priv- >bookmarks, node); + g_list_free (selection); + } + } Why you are not using just ephy_node_view_remove here ? +static void +cmd_copy (EggAction *action, + EphyBookmarksEditor *editor) +{ + GtkWidget *widget = gtk_window_get_focus (GTK_WINDOW (editor)); + + if (GTK_IS_EDITABLE (widget)) + { + gtk_editable_copy_clipboard (GTK_EDITABLE (widget)); + } +} + +static void +cmd_paste (EggAction *action, + EphyBookmarksEditor *editor) +{ + GtkWidget *widget = gtk_window_get_focus (GTK_WINDOW (editor)); + + if (GTK_IS_EDITABLE (widget)) + { + gtk_editable_paste_clipboard (GTK_EDITABLE (widget)); + } +} Align borked. I'm not completely sure if get_focus can return NULL and if GTK_IS_EDITABLE can handle that case. Could be worth to check. + } + + else if (GTK_IS_TREE_VIEW (widget)) + { + GtkTreeSelection *sel = gtk_tree_view_get_selection (GTK_TREE_VIEW (widget)); + gtk_tree_selection_select_all (sel); + } No new line. I think it's better to add ephy_node_select_all instead of using the widget directly here. + case GDK_Delete: + + selection = ephy_node_view_get_selection (editor- >priv->key_view); + + if (selection) + { + EphyNode *node = EPHY_NODE (selection->data); + ephy_bookmarks_remove_keyword (editor->priv- >bookmarks, node); + g_list_free (selection); + } + break; Why not ephy_node_view_remove ? + if (gtk_tree_view_get_path_at_pos (tree_view, + (gint) event->x, + (gint) event->y, + &path, NULL, + NULL, NULL)) Align ... + g_signal_connect (G_OBJECT(treeview), "button_press_event", + G_CALLBACK (topic_clicked), editor); Align ... Damn I have to leave now. I'll be able to review the fixed patch only tomorrow or after tomorrow, sorry :(
> Here you borked all align ;) I fixed gedit to use monospace fonts so it should be able to fix the align now. :) > Why you are not using just ephy_node_view_remove here ? That was in the code that I copied and pasted, i'll fix it to use view_remove. >No new line. >I think it's better to add ephy_node_select_all instead of using the >widget directly here. ok >I'm not completely sure if get_focus can return NULL and if >GTK_IS_EDITABLE can handle that case. Could be worth to check. get_focus can return NULL, fwiw i cut and pasted that code verbatim from window-commands.c Oh and no rush....
>get_focus can return NULL, fwiw i cut and pasted that code verbatim >from window-commands.c gtype checks for NULL ... so nm, it's not necessary >That was in the code that I copied and pasted, i'll fix it to use >view_remove. You mentioned in the mail we was removing also with no selection. I'm a bit confused now but ... maybe the bug is in view_remove. Oh if you know where you cut and pasted that code from, it may be worth to fix that too (prolly it has been wrote before I added that func).
> You mentioned in the mail we was removing also with no selection. I'm > a bit confused now but ... maybe the bug is in view_remove. well not with no selections, but we were removing something that had secondary selection. example: 1. click a topic 2. click a bookmark within the topic (giving the bookmark primary selection color and topic secondary selection color(selected but unfocus)). 3. edit -> delete topic, well the topic gets deleted, but that is completely unusually since only items that are the primary selected items should acted upon by menu items. make sense so you would still need to do ephy_view_has_focus checks on delete/rename topic/bookmark...but i think single menu items are better anyway. > Oh if you know where you cut and pasted that code from, it may be > worth to fix that too (prolly it has been wrote before I added that > func). You mean view_remove? It was in ephy_bookmarks_editor.c (remove/delete topic functions), so its fixed now in my upcoming patch.
Created attachment 15214 [details] [review] lets give a hand for monospace fonts in gedit
Just some minor notes so you know what else is on my radar: 1. in the new bookmark and properties window, i would like to use space and enter to add/remove keywords. however I ran into a lot of trouble when doing this (wasted a ton of time and felt defeated). Any suggestions? the code I used button_clicked i stole acme, but i couldn't figure out how he did the keyboard stuff. 2. keyword_node_key_pressed_cb should probably be generified to be used by both the bm and key views. (one callback instead of two) 3. We need to fix the show_popup_cb in the editor so that we show the generic text entry context menu when in editing mode. I'd like to commit the above patch first though before doing any of this stuff....
static void +keyword_node_key_pressed_cb (GtkWidget *view, + GdkEventKey *event, + EphyBookmarksEditor *editor) Alignement is still borked here and in a lot of functions before this in that way. I thought a bit to the pane thing. Do we really want it ? It seem like the only size and user would want is the size of his longer topic. Couldnt we just automatically make it of the right size ? Am I missing something ? I'll prolly leave now. If you are able to fix alignement of the functions please commit that part. I'd prefer we discuss pane thing before committing that part though.
About things on your radar 1 gtk_treeview_get_selection gtk_tree_selection_get_selected gtk_tree_model_get_path Then convert this to path_str and do like for click 2-3 Yeah they sounds good.
grumble, grumble, /me swore i fixed all the aligns...ugghhh.... I really like the pane actually. This is how I look at it. The pane provides users with a little more leeway in how the window works without any added ui complexity. For instance if a user (for some reason wants a really small window they can adjust the pane to there preference). I guess i see some nominal ui benefit without any extra ui complexity.
Created attachment 15230 [details] [review] I checked this in, even used emacs to check the aligns. Please smack me over the head if i screwed this up.
Created attachment 15233 [details] [review] here's my keynav patch i just sent to ephy list.
> 3. We need to fix the show_popup_cb in the editor so that we show the > generic text entry context menu when in editing mode. I looked at the list view code in nautilus today. I think to do this right, we're going to need to grab the click, check what button is clicked and if we are in rename mode. If button 3 && !in_rename_mode, show the popup. Also, dave just added some drag and drop code to the nautilus list view that lets you click+drag in one action from the list. I talked to him about it, apparently the code is somewhat complex, and he had to implement the dnd code himself (as opposed to using egg_multidnd stuff). Should i look into putting this code into the ephy_node_view too?
+static void +cmd_select_all (EggAction *action, + EphyBookmarksEditor *editor) E should be under E not under ( Anyway not a big deal, just to let you know (no need to fix it now). >I really like the pane actually. This is how I look at it. The pane >provides users with a little more leeway in how the window works >without any added ui complexity. For instance if a user (for some >reason wants a really small window they can adjust the pane to there >preference). I guess i see some nominal ui benefit without any extra >ui complexity. I'm not sure I see the benefit, if you want a small window you would prolly use short topics ... cutted topics looks very bad ihmo (we add some ui complexity, even if minimal). I'm a bit worried this would not let us implement automatic sizing too. >Should i look into putting this code into the ephy_node_view too? Would rock, that's a bad usability issue ihmo.
The kb patch is good. Please just fix the alignement as I explained in my previous comment. (Then commit it, no need to review again)
>E should be under E not under ( >Anyway not a big deal, just to let you know (no need to fix it now). Uggh...i'm totally perplexed here. in both emacs and gedit (with monospace fonts) the alignment looks right,but on bonsai its all fucked, of course all the alignments on bonsai look wrong (even the one's i didn't do). Should I just use spaces (as opposed to tabs) in the future? >>Should i look into putting this code into the ephy_node_view too? >Would rock, that's a bad usability issue ihmo. God this code is complex nautilus/src/file-manager/fm-list-view.c if anyone else is interested. >I'm a bit worried this would not let us implement automatic sizing >too. Why would you ever want to automatically resize? Are you refering to the window itself? Resizing the window after its drawn is probably a bad idea.... also just from everyday use, right now i have topics that get cut off in the list, hence the reason i like the hpane. Setting the topics list to expand and fill just doesn't look right though using an hbox.
i'm going to marked this fixed. the other issues have open bugs anyway.
>Uggh...i'm totally perplexed here. in both emacs and gedit (with >monospace fonts) the alignment looks right,but on bonsai its all >fucked, of course all the alignments on bonsai look wrong (even the >one's i didn't do). Should I just use spaces (as opposed to tabs) in >the future? No no, use tabs ;) Maybe bonsai doesnt use monospace ? >God this code is complex nautilus/src/file-manager/fm-list-view.c if >anyone else is interested. Argh, I wonder when there will be a real solution for this. We should prolly open a bug about it .... >Why would you ever want to automatically resize? Are you refering to >the window itself? Resizing the window after its drawn is probably a >bad idea.... No no :) I mean we should automatically size the left treeview so that it's exactly big as needed (== the size of the longer topic). >also just from everyday use, right now i have topics that get cut off >in the list, hence the reason i like the hpane. Setting the topics >list to expand and fill just doesn't look right though using an hbox. I think jorn had some code in rb to have smart sizing ... If we cant do that obviously we should fall back to the pane. But if we can get that right, would you be ok ?
> Argh, I wonder when there will be a real solution for this. We should > prolly open a bug about it .... there was rumour of a potential modes api in gtk 2.4, but i haven't seen too much comment on gtk-devel about it (though i don't follow that list too closely). > No no :) I mean we should automatically size the left treeview so > that it's exactly big as needed (== the size of the longer topic). > I think jorn had some code in rb to have smart sizing ... If we cant > do that obviously we should fall back to the pane. But if we can get > that right, would you be ok ? I guess without testing i can't give a definitive yes/no, but it definately sounds reasonable. Caveat is that i think having the list change size just because a topic's length changes could be somewhat distracting. A good example of this is the icon column in the bm view. If a topic you are viewing has no bm's with icons and than you switch to a topic that does have bm's with icon, the adjustment of the icon column is somewhat distracting. (but this can be fixed pretty easily i think.)