jverzani / gWidgets2

Rewrite of gWidgets
35 stars 9 forks source link

nasty DnD bug that crashes R #74

Closed landroni closed 10 years ago

landroni commented 10 years ago

Try the following:

require('gWidgets2RGtk2')
w <- gwindow()
vb <- gvbox(container=w)
data_set_nms <- names(mtcars)
search_type <-  list(ignore.case=TRUE, perl=FALSE, fixed=FALSE)  ##init global instance
gp <- ggroup(cont=vb)
ed <- gedit("", initial.msg="Filter values by...", expand=TRUE, container=gp)
ed$set_icon("ed-search", "start")
ed$set_icon("ed-remove", "end")
ed$set_icon_handler(function(h,...) {
    svalue(ed) <- ""
    focus(ed) <- TRUE
}, where="end")
ed$widget$setIconActivatable("primary", FALSE)

search_handler <- function(h,..., do_old=TRUE) {
    ## we keep track of old selection here
    ## that updates only when user changes selection, not when filter does
    #cur_sel <- old_selection
    blockHandlers(tbl)
    on.exit(unblockHandlers(tbl))
    val <- svalue(ed)

    if(val == "") {
        tbl[] <<- data_set_nms
        ed$widget$modifyBase(GtkStateType["normal"], NULL)
        ed$widget$modifyText(GtkStateType["normal"], NULL) 
    } else {
        l <- c(list(pattern=val, x=data_set_nms), search_type)
        new_vals = data_set_nms[do.call(grepl, l)]
        if (length(new_vals)) {
            tbl[] <<- new_vals
            ed$widget$modifyBase(GtkStateType["normal"], NULL)
            ed$widget$modifyText(GtkStateType["normal"], NULL) 
        } else {
            tbl[] <<- character(0) 
            ed$widget$modifyBase(GtkStateType["normal"], "#FF6666")
            ed$widget$modifyText(GtkStateType["normal"], "white") 
            return()
        }
    }
    #svalue(tbl) <<- cur_sel
}

b <- gbutton("", cont=gp)
tooltip(b) <- "Search options"
b$set_icon("properties")

cbs <- list(gcheckbox("Ignore case", checked=TRUE, handler=function(h,...) {
    search_type[["ignore.case"]] <<- svalue(h$obj)
    search_handler(do_old=FALSE)
}),
gcheckbox("Regex", checked=TRUE, handler=function(h,...) {
    search_type[["fixed"]] <<- !svalue(h$obj)
    search_handler(do_old=FALSE)                                                     
}),
gcheckbox("Perl compatible", checked=FALSE, handler=function(h,...) {
    search_type[["perl"]] <<- svalue(h$obj)
    search_handler(do_old=FALSE)                                                     
})
)

addPopupMenu(b, gmenu(cbs, popup=TRUE))

addHandlerKeystroke(ed, search_handler)
addHandlerChanged(ed, search_handler)

tbl <- gtable(names(mtcars), cont=vb)

addDropSource(tbl, handler=function(h,...){
    svalue(tbl)
})

gedit(cont=vb)

Now:

Now for the part that crashes R:

Here's the trace that I get:

*** caught segfault ***
    address (nil), cause 'memory not mapped'

Traceback:
    1: .Call(name, ..., PACKAGE = PACKAGE)
2: .RGtkCall("S_gtk_selection_data_set_text", object, str, len,     PACKAGE = "RGtk2")
3: method(obj, ...)
4: sel$setText(val, -1)
5: (function (h, widget, context, sel, tType, eTime) {    val <- handler(h)    sel$setText(val, -1)})(list(obj = <S4 object of class "GTable">, action = NULL),     <pointer: 0x6c78460>, <pointer: 0x6cf6b40>, <pointer: 0x7fff1a19e3c0>,     0L, 1420138206L)
jverzani commented 10 years ago

I just pushed a check against 0-length strings. What I suspect you want is something different -- actually dropping the value. This is an issue, as svalue is character(0) unless there is a selection and this set of commands doesn't do that. I'm not sure why, but the first click focuses the window, but does not select the value.

jverzani commented 10 years ago

I can get the behavior you likely want. Add this to your callbacks:

addHandler(tbl, "enter-notify-event", function(h,...) {
focus(h$obj) <- TRUE
FALSE
})
landroni commented 10 years ago

Thanks a lot for this! You're right on what I'd like to achieve. The "enter-notify-event" handler allows me to do almost exactly what I wanted, but I still have to experiment a little bit.

landroni commented 10 years ago

"I'm not sure why, but the first click focuses the window, but does not select the value."

I played around with the code, and I think I pinpointed the offending code: it seems to me that something fishy happens on [<- on the gtable (or gcheckboxgroup) object.

Here's a much simplified reproducible example:

require('gWidgets2RGtk2')
w <- gwindow()
vb <- gvbox(container=w)
data_set_nms <- names(mtcars)
gp <- ggroup(cont=vb)
ed <- gedit("", initial.msg="Filter values by...", expand=TRUE, container=gp)

search_handler <- function(h,..., do_old=TRUE) {
    ## we keep track of old selection here
    ## that updates only when user changes selection, not when filter does
    #cur_sel <- old_selection
    blockHandlers(tbl)
    on.exit(unblockHandlers(tbl))
    val <- svalue(ed)

    if(val == "") {
        tbl[] <<- data_set_nms  ##FIXME experiment by commenting this line out
    }
}

addHandlerKeystroke(ed, search_handler)
addHandlerChanged(ed, search_handler)

tbl <- gtable(names(mtcars), cont=vb)

addDropSource(tbl, handler=function(h,...){
    svalue(tbl)
})

addHandler(tbl, "enter-notify-event", function(h,...) {
    focus(h$obj) <- TRUE
    FALSE
})

gedit(cont=vb)

The main difference between the two gedit boxes is that the 1st one has a handler which replaces the items: tbl[] <<- data_set_nms. If you comment that out, the offending behaviour disappears. If you don't, then you should see this:

You won't notice the same behaviour with the 2nd gedit if you redo the steps above (or if you comment out the [<- line in the 1st gedit).

Can anything be done to avoid re-scrolling on focus of gtable after a [<- assignment?

jverzani commented 10 years ago

The issue is you are changing the underlying data with the [<- assignment, and there is no default selection. Try adding something like:

if(val == "") { ind <- svalue(tbl, index=TRUE) tbl[] <<- data_set_nms ##FIXME experiment by commenting this line out svalue(tbl, index=TRUE) <- ind }

This still isn't perfect, as the selection will remain, but the window will still scroll. For that you might try this code:

https://mail.gnome.org/archives/gtk-perl-list/2005-September/msg00069.html sel = tbl$widget$getSelection() gSignalConnect(sel, "changed", function(sel) { l <- sel$getSelected() model <- l$model iter <- l$iter if (l$retval) { sel$getTreeView()$scrollToCell(model$getPath(iter)) } })

--J

On Sat, Aug 16, 2014 at 2:20 PM, landroni notifications@github.com wrote:

"I'm not sure why, but the first click focuses the window, but does not select the value."

I played around with the code, and I think I pinpointed the offending code: it seems to me that something fishy happens on [<- on the gtable (or gcheckboxgroup) object.

Here's a much simplified reproducible example:

require('gWidgets2RGtk2') w <- gwindow() vb <- gvbox(container=w) data_set_nms <- names(mtcars) gp <- ggroup(cont=vb) ed <- gedit("", initial.msg="Filter values by...", expand=TRUE, container=gp)

search_handler <- function(h,..., do_old=TRUE) {

we keep track of old selection here

## that updates only when user changes selection, not when filter does
#cur_sel <- old_selection
blockHandlers(tbl)
on.exit(unblockHandlers(tbl))
val <- svalue(ed)
if(val == "") {
    tbl[] <<- data_set_nms  ##FIXME experiment by commenting this line out
}

}

addHandlerKeystroke(ed, search_handler) addHandlerChanged(ed, search_handler)

tbl <- gtable(names(mtcars), cont=vb)

addDropSource(tbl, handler=function(h,...){ svalue(tbl) })

addHandler(tbl, "enter-notify-event", function(h,...) { focus(h$obj) <- TRUE FALSE })

gedit(cont=vb)

The main difference between the two gedit boxes is that the 1st one has a handler which replaces the items: tbl[] <<- data_set_nms. If you comment that out, the offending behaviour disappears. If you don't, then you should see this:

  • Focus gtable obj, scroll down and select carb
  • Focus 1st gedit obj (click to put in the cursor)
  • Then in the WM minimize and un-minimize window (or just hit alt or ctrl key)
  • Notice that the selection was reset, and more disturbingly the gtable has been automatically scrolled to the beginning.

You won't notice the same behaviour with the 2nd gedit if you redo the steps above (or if you comment out the [<- line in the 1st gedit).

Can anything be done to avoid re-scrolling on focus of gtable after a [<- assignment?

— Reply to this email directly or view it on GitHub https://github.com/jverzani/gWidgets2/issues/74#issuecomment-52401695.

John Verzani Chair, Department of Mathematics College of Staten Island, CUNY verzani@math.csi.cuny.edu

landroni commented 10 years ago

Thanks! I'll play around with this..

landroni commented 10 years ago

Hmm, I played around a bit and I think I found a solution (still trying to tweak it to perfection), but this got me thinking: Isn't this actually a bug in the way addHandlerKeystroke and addHandlerChanged behave?

If you take the simplified code in https://github.com/jverzani/gWidgets2/issues/74#issuecomment-52401695, is there a good reason for either of the two handler events to activate the handler when simply focusing with the mouse the ed input box? I mean, there is no change in the value of ed, so why would then the handler get activated?

jverzani commented 10 years ago

I don't know, but the keystroke handler utilizes key-release-event which seems like the right thing to do. It would be a Gtk decision I would guess, I'm not doing anything special with gWidgets2RGtk2 as far as I can tell. --J

On Wed, Aug 20, 2014 at 1:53 PM, landroni notifications@github.com wrote:

Hmm, I played around a bit and I think I found a solution (still trying to tweak it to perfection), but this got me thinking: Isn't this actually a bug in the way addHandlerKeystroke and addHandlerChanged behave?

If you take the simplified code in #74 (comment) https://github.com/jverzani/gWidgets2/issues/74#issuecomment-52401695, is there a good reason for either of the two handler events to activate the handler when simply focusing with the mouse the ed input box? I mean, there is no change in the value of ed, so why would then the handler get activated?

— Reply to this email directly or view it on GitHub https://github.com/jverzani/gWidgets2/issues/74#issuecomment-52815508.

John Verzani Chair, Department of Mathematics College of Staten Island, CUNY verzani@math.csi.cuny.edu

landroni commented 10 years ago

But then does a mouse click register as a key-release-event? Here I can trigger the offending behavior (i.e. addHandlerKeystroke activates the handler) when I simply focus the 1st input box then focus the 2nd input box, all with the mouse and having pressed no keyboard key. Is this expected behavior?

jverzani commented 10 years ago

Okay, for sure the key release event is not triggered by the mouse click, as you surmised.

So maybe the issue is with this code in the gedit constructor:

add_handler_blur(function(...) invoke_change_handler())

This causes the edit widget's change handler to be called when you click outside of the edit area. I have this as the documented behavior. Likely you can avoid it by using addHandler(ed, "activate", ...) in place of addHandlerChanged?

landroni commented 10 years ago

Yes, this does sound like the behavior of add_handler_blur().

I tried with addHandler(ed, "activate", search_handler), but the behavior is unchanged: I still get the handler activated upon focusing ed. Maybe another signal? (BTW, are these documented?)

landroni commented 10 years ago

This causes the edit widget's change handler to be called when you click outside of the edit area. I have this as the documented behavior.

But is this behavior desirable in all use cases, I wonder? Maybe this could be made optional: activate.handler.on.unfocus=TRUE? (Or any other less verbose argument name.)

jverzani commented 10 years ago

Okay, optional it is. A user now must call addHandlerBlur to get that behavior. Hopefully this works for you. Let me know if not.

On Wed, Aug 20, 2014 at 3:20 PM, landroni notifications@github.com wrote:

This causes the edit widget's change handler to be called when you click outside of the edit area. I have this as the documented behavior.

But is this behavior desirable in all use cases, I wonder? Maybe this could be made optional..

— Reply to this email directly or view it on GitHub https://github.com/jverzani/gWidgets2/issues/74#issuecomment-52829861.

John Verzani Chair, Department of Mathematics College of Staten Island, CUNY verzani@math.csi.cuny.edu

jverzani commented 10 years ago

I'm pretty sure I cribbed it from http://qt-project.org/doc/qt-4.8/qlineedit.html#editingFinished but what you suggested is better, as it is easier to work around.

On Wed, Aug 20, 2014 at 3:26 PM, John Verzani jverzani@gmail.com wrote:

Okay, optional it is. A user now must call addHandlerBlur to get that behavior. Hopefully this works for you. Let me know if not.

On Wed, Aug 20, 2014 at 3:20 PM, landroni notifications@github.com wrote:

This causes the edit widget's change handler to be called when you click outside of the edit area. I have this as the documented behavior.

But is this behavior desirable in all use cases, I wonder? Maybe this could be made optional..

— Reply to this email directly or view it on GitHub https://github.com/jverzani/gWidgets2/issues/74#issuecomment-52829861.

John Verzani Chair, Department of Mathematics College of Staten Island, CUNY verzani@math.csi.cuny.edu

John Verzani Chair, Department of Mathematics College of Staten Island, CUNY verzani@math.csi.cuny.edu

landroni commented 10 years ago

This does work! Thanks.

As for the old behavior, I think it was useful in other contexts. For example latticist uses it throughout the GUI to auto-generate the graphs once the selection was made and the input box got unfocused. So I would suggest to prominently advertise this change when a new release is made, and document addHandlerBlur in ?gedit as a way to trigger the old behavior. Now that it's optional people may have a hard time figuring how to obtain it.

jverzani commented 10 years ago

Good idea. I just added some docs to the main ?gedit page. Before it was buried in the addHandler docs.

On Wed, Aug 20, 2014 at 3:37 PM, landroni notifications@github.com wrote:

This does work! Thanks.

As for the old behavior, I think it was useful in other contexts. For example latticist uses it throughout the GUI to auto-generate the graphs once the selection was made and the input box got unfocused. So I would suggest to prominently advertise this change when a new release is made, and document addHandlerBlur in ?gedit as a way to trigger the old behavior. Now that it's optional people may have a hard time figuring how to obtain it.

— Reply to this email directly or view it on GitHub https://github.com/jverzani/gWidgets2/issues/74#issuecomment-52832171.

John Verzani Chair, Department of Mathematics College of Staten Island, CUNY verzani@math.csi.cuny.edu

landroni commented 10 years ago

Looks nice. In any case thanks a lot for this fix: now I can scratch an obscure UI issue from the TODO list..

jverzani commented 10 years ago

Happy to help.

On Wed, Aug 20, 2014 at 3:46 PM, landroni notifications@github.com wrote:

Looks nice. In any case thanks a lot for this fix: now I can scratch an obscure UI issue from the TODO list..

— Reply to this email directly or view it on GitHub https://github.com/jverzani/gWidgets2/issues/74#issuecomment-52833315.

John Verzani Chair, Department of Mathematics College of Staten Island, CUNY verzani@math.csi.cuny.edu