Streamlining the WRootWin API

[buhl]

I was creating a function ioncore.find_current modeled on ioncore.find_manager for something I am playing around with.

--DOC
-- Find a current object with type name \var{t} under \var{obj} or one of
-- its current objects.
function ioncore.find_current(obj, t)
    while obj~=nil do
        if obj_is(obj, t) then
            return obj
        end
        -- obj=obj:current()
        if obj_is(obj, "WRootWin") then
            obj=obj:current_scr()
        else
            obj=obj:current()
        end
    end
end

As you can see, I came across an oddity. WRootWin.current returned nil. It seemed I have to use WRootWin.current_scr.

I did a quick grep to see where rootwin_current_scr was used which was only in ioncore/fullscreen.c

It seems weird that the API is not uniform so I renamed rootwin_current_scr to rootwin_current. I might be missing some pieces of the puzzle but this fix seems to work fine. Are there any reasons why current_scr was chosen?

On a side-note. I am close to a C noob and this line makes very little sense to me. Could someone explain what is going on? https://github.com/raboof/notion/blob/67174dd4865a65b99509365fd2712aa4b38441a0/ioncore/region.c#L247

[buhl]

So I think understand the CALL_DYN_RET part now after finding this part in the documentation. I think this part https://github.com/raboof/notion/blob/67174dd4865a65b99509365fd2712aa4b38441a0/ioncore/mplex.c#L2060-L2061 and this part https://github.com/raboof/notion/blob/67174dd4865a65b99509365fd2712aa4b38441a0/ioncore/group.c#L1372-L1373 are overloading WRegion.current? so maybe my fix should have been to add rootwin_current_scr here https://github.com/raboof/notion/blob/67174dd4865a65b99509365fd2712aa4b38441a0/ioncore/rootwin.c#L492-L502 to overload WRootWin.current

[buhl]

So this actually seemed to do the trick. Getting deeper and deeper into the code base :safety_vest:

[raboof]

(this looks reasonably, but I'll do some digging in the history to validate)

[buhl]

Been trying to look into this git log -L :rootwin_dynfuntab:ioncore/rootwin.c showed that WRootWin never had a current DYNFUN and there have never been a rootwin_current C function.

Looking at the inheritance graph

 Obj
  |-->WRegion
       |-->WWindow
            |-->WRootWin

WWindow has no current and WRegion current looks like it is just a wrapper for a DYNFUN. https://github.com/raboof/notion/blob/77c6ddf5a25b56f8a9383e877f61bcb30ad1a535/ioncore/region.c#L244-L249 TBH I am not sure I fully understand that snippet of code. And I am also unclear on what happens when there are no current function defined higher up in the hierarchies?

I am tempted to think that :current has never worked for WRootWin which seems odd when rootwin_current_scr has been there since 2003.

@raboof when you say digging in the history, what does that entail (Just so I can maybe help with it some other time)

[buhl]

@raboof have you had time to look into this?

[wilhelmy]

Is there ever more than one WRootWin? I had the impression that in modern-day multihead setups, there was only one root window spanning all screens. Therefore, this one is always the current one... please correct me if I'm wrong. Edit: I'm wondering if that's why it went unnoticed. I still think we should merge a bugfix patch, if it fixes a bug.

[raboof]

when you say digging in the history, what does that entail

What I meant was that I wanted to look at the commit where current_scr was implemented, and see if I could find a clue why it wasn't implemented as an override of the current on WRegion.

Looking at 90c3edc313a142f699e6a0d01f5c669a4da4f4cb, this function used to be called screen_current_vp. The old screen was also a subclass of WWindow (and thus WRegion). However, at that time there was no current in WRegion yet: that wasn't introduced until d03c5730e75f8ad41195a74fc7b24f74c198a0bb .

So with that background it indeed seems to be an oversight that the current_scr on WRootWin wasn't just an overload of the current on WRegion.

[raboof]

Is there ever more than one WRootWin?

No, but current_scr on a WRootWin doesn't give the the current WRootWin, it gives the current WScreen under the WRootWin.

I still think we should merge a bugfix patch, if it fixes a bug.

I don't think this was a bug, but I certainly think this is an improvement, so we should merge it as such ;)

[raboof]

Looking at the inheritance graph

 Obj
  |-->WRegion
       |-->WWindow
            |-->WRootWin

WWindow has no current and WRegion current looks like it is just a wrapper for a DYNFUN.

https://github.com/raboof/notion/blob/77c6ddf5a25b56f8a9383e877f61bcb30ad1a535/ioncore/region.c#L244-L249

TBH I am not sure I fully understand that snippet of code.

What this means is: "look at the dynfuntab of the run-time type of mgr, and go up the inheritance hierarchy until you find a type that defines an implementation of region_current. When you find one, call it, with parameters (mgr), and assign the result to ret.

And I am also unclear on what happens when there are no current function defined higher up in the hierarchies?

As you can see in the implementation at https://github.com/raboof/notion/blob/master/libtu/objp.h#L70-71, if there is no function found, nothing is assigned to RETV. So in that case ret will keep the NULL value it was originally initialized with.

[buhl]

Thanks @raboof for taking the time to explain. I have little experience with C so this codebase is filled with learning opportunities :smile: