New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tool palette does not move with window focus/use #3397

Open
skef opened this Issue Jan 6, 2019 · 16 comments

Comments

Projects
None yet
3 participants
@skef
Copy link

skef commented Jan 6, 2019

As I've been looking at other problems I have compiled FontForge with debugging turned on and sort of half-assed my build environment. All that time I've been having one strange problem that I attributed to a poor build. Now that things are closer to release I decided to replicate the OS's build as closely as I could and see if the problem remained.

It did, so I figured I should open an issue about it.

Environment: I use Arch Linux with its standard X server and KDE5 Plasma. I synced with the current Arch repository before this latest testing. It provides a version of FontForge it calls 20170731-2 that doesn't have the problem.

Method to reproduce and expected behavior (based on the 20170731-2 package):

Open a font file and then a glyph outline window. The tool palette appears in the latter. Now open a second glyph outline window and the tool palette disappears from the first and appears in the second.
Now foreground the first outline window and the tool palette does not shift back immediately, but does shift once you click anywhere in the window. In this way you can move the the palette back and forth between windows.

(Note: this is actually a bit of a pain if you have a tool other than the arrow selected, because you can accidentally create points trying to get the palette back, but fine if you remember to click in the ruler area.)

The problem:

When I compile the current git source (plus my viewBox push -- but I've been seeing this problem for a couple months with different fixes) and follow the procedure above, the tool palette always stays in the first window. Here is a screenshot taken after repeatedly clicking in the "C" window:
ow

I can still switch between tools by going back to the first window or with the menu, but obviously it's a pain to use the program this way.

Note that I also get these warnings that I doubt are related but could be:

Traceback (most recent call last):
  File "/usr/share/fontforge/python/graphicore.py", line 25, in <module>
     import graphicore.shell
  File "/usr/share/fontforge/python/graphicore/shell.py", line 26, in <module>
    import gtk
  ModuleNotFoundError: No module named 'gtk'

I've tried to fix this in the past but I haven't found a combination of gtk and python 3 in Arch that makes FontForge "happy".

@skef

This comment has been minimized.

Copy link

skef commented Jan 6, 2019

Oh, I also never see any "tooltips", which it occurs to me might also be relevant.

@skef

This comment has been minimized.

Copy link

skef commented Jan 6, 2019

Direct versions of dependencies:

  1. libtool: 2.4.6+42+gb88cebd5-2
  2. libxkbui: 1.0.2-6
  3. libxi: 1.7.9-2
  4. freetype2: 2.9.1-1
  5. pango: 1.42.4-1
  6. giflib: 5.1.4-2
  7. libxml2: 2.9.8-6
  8. libspiro: 0.5.20150702-2
  9. libunicodenames: 1.2.2-1
  10. zeromq: 4.3.0-1
  11. python: 3.7.2-1
  12. desktop-file-utils: 0.23+4+g92af410-1
  13. hicolor-icon-theme: 0.17-1

However, as these are the same versions used by the Arch-provided version it's not obvious how these would contribute to the problem.

@ctrlcctrlv

This comment has been minimized.

Copy link

ctrlcctrlv commented Jan 7, 2019

Why did you mention GTK? Are you sure you're using the GTK patch?

@skef

This comment has been minimized.

Copy link

skef commented Jan 7, 2019

Why did you mention GTK? Are you sure you're using the GTK patch?

I only mentioned GTK because of the graphicore.py-related warning messages. I don't have any reason to think it is related.

And I'm not sure I'm using the GTK patch -- I'm just using the Arch Linux-supplied environment. Could you point me to the docs for that?

@skef

This comment has been minimized.

Copy link

skef commented Jan 7, 2019

OK, I just compiled a debug version based on tag 20170731 (without python -- that compiled but had execution issues with 3.7) and it doesn't have the problem. One doesn't get much better than this from a debugging perspective, so I'll do some stepping and nexting and try to nail down the relevant difference.

@skef

This comment has been minimized.

Copy link

skef commented Jan 7, 2019

@jtanx : As of tag 20170731 the code had this function:

static void ReparentFixup(GWindow child,GWindow parent, int x, int y, int width, int height ) {
    /* This is so nice */
    /* KDE does not honor my request for a border for top level windows */
    /* KDE does not honor my request for size (for narrow) top level windows */ 
    /* Gnome gets very confused by reparenting */
      /* If we've got a top level window, then reparenting it removes gnome's */
      /* decoration window, but sets the new parent to root (rather than what */
      /* we asked for */
      /* I have tried reparenting it twice, unmapping & reparenting. Nothing works */

    GWidgetReparentWindow(child,parent,x,y);
    if ( width!=0 )
      GDrawResize(child,width,height);
    GDrawSetWindowBorder(child,1,GDrawGetDefaultForeground(NULL));
}

which was called in _CVPaletteActivate() like this:

     if ( palettes_docked ) {
        ReparentFixup(cvtools,cv->v,0,0,getToolbarWidth(cv),getToolbarHeight(cv));
        if ( cv->b.sc->parent->multilayer )
            ReparentFixup(cvlayers2,cv->v,0,getToolbarHeight(cv)+2,0,0);
        else
            ReparentFixup(cvlayers,cv->v,0,getToolbarHeight(cv)+2,0,0);

Your commit 50cc555 on 2016-04-16 eliminated that function and replaced the other code with:

if (palettes_docked) {
    if (cvvisible[1])
        GDrawRequestExpose(cvtools, NULL, false);
    if (cvvisible[0]) {
        if (cv->b.sc->parent->multilayer)
            GDrawRequestExpose(cvlayers2, NULL, false);
        else
            GDrawRequestExpose(cvlayers, NULL, false);
    }

The problem seems to stem from this change. I've looked at this a bit but there is enough change that I have no immediate ideas for a fix. Any further debugging advice?

@jtanx

This comment has been minimized.

Copy link
Contributor

jtanx commented Jan 7, 2019

Yeah sounds familiar, the reparenting stuff was removed because it's not supported by the GDK backend. I don't have any particular advice at the moment, I'd have to re-familiarise with this stuff.

@skef

This comment has been minimized.

Copy link

skef commented Jan 7, 2019

Is anyone "listening" in a position to follow the steps to reproduce above with the current code (or relatively recent) on Linux and X? If this is general to Linux (or more general than that) it seems like it would probably be a release-blocker.

@jtanx

This comment has been minimized.

Copy link
Contributor

jtanx commented Jan 7, 2019

I can repro, will try to take a look, but I am short on time atm

P.S. If you look hard enough, there are more than a few gui quirks...

@jtanx jtanx added I-bug UI labels Jan 7, 2019

@skef

This comment has been minimized.

Copy link

skef commented Jan 7, 2019

An initial assessment of the code by more detailed inspection:

fontforgeexe/cvpalettes.c has a static GWIndow global called cvtools that points to the tools palette (sub)window. It is initialized to non-NULL more or less once in CVMakeTools() in the same file and is not deleted in the case of (for example) a window focus switch.

fontforgeexe/cvpalettes.c:_CVPaletteActivate() is the function called on a window to ensure the palette is shown when a new outline window is used. The old code called ReparentFixup() as quoted above. The new code calls GDrawRequestExpose() instead. From what I can tell there is now nothing in the new code path to associate the cvtools window with any window other than the one it was created for. If that is right then this bug will reproduce the same way on every platform.

If "reparenting" is really off the table due to GDK limitations, I presume the most straightforward fix would be to move those static GWindow entries into struct CharView and have one palette per outline window. In that case the question is now much damage is involved in having multiple palettes. The state of the palette is currently managed with GDrawSetUserData and is per-window, so that should be easy. And if every CharView had its own palette SubWindow a lot of this activation stuff would probably be superfluous. So I'm guessing the trick, and potential mess, would be getting the event wiring right. I have Literally No Idea how that works now, so I guess I'll further my self-destructive life path this evening by looking at that a little.

@skef

This comment has been minimized.

Copy link

skef commented Jan 7, 2019

P.S. If you look hard enough, there are more than a few gui quirks...

I've been using the program in this configuration on and off for a couple months. The "main" tools have keystrokes but others, including the point type tools don't (or don't obviously), which means you have to pick them from the annoying Point -> Tools submenu. Given that many common use-cases involve multiple windows I don't think it's viable to release in this state. The Outline window Tabs system would be a possible work-around but it barely has an interface.

@jtanx

This comment has been minimized.

Copy link
Contributor

jtanx commented Jan 7, 2019

I like your determinism, gdraw is not fun to work with. I'll take a look at that ReparentFixup stuff, there may be something to do there instead.

@skef

This comment has been minimized.

Copy link

skef commented Jan 7, 2019

Actually, there is a viable short term workaround if needed: As far as I can tell everything works as expected in the current version with undocked palettes. So in a pinch that could be made the default and the opposite could even be disabled while a better fix is in progress.

@jtanx

This comment has been minimized.

Copy link
Contributor

jtanx commented Jan 7, 2019

I have a rough idea of what to do, basically if we detect that another window has activated, we destroy the old cvpalettes and recreate it. We already do that when we changed from docked to non-docked mode and vice/versa so in theory it should be fine

Reusing and reparenting the window was a dumb idea...

@skef

This comment has been minimized.

Copy link

skef commented Jan 7, 2019

basically if we detect that another window has activated, we destroy the old cvpalettes and recreate it.

Ah, yes, that does make a lot of sense.

jtanx added a commit to jtanx/fontforge that referenced this issue Jan 7, 2019

cvpalettes.c: Recreate cvtools/bvtools each time it needs to be assoc…
…iated with a new window

Broken since the reparenting stuff was removed. Having a single set of
palette tools seems like a bad idea...

Fixes fontforge#3397
@skef

This comment has been minimized.

Copy link

skef commented Jan 7, 2019

I'm not up enough on the bitmap stuff to easily test that code path, but I brought over and tested these changes in outline view. This code works like the tag 20170731 code in almost all ways, and works better in one way: the palette now appears immediately when the window gets focus.

@skef skef referenced this issue Jan 14, 2019

Merged

Re-enable tooltips (aka "popup windows") in GXdraw #3413

3 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment