Remap user dialog - where to go digging to make this resizable?

[Harleya]

No worries. And I can always help you out if you are not sure what to do or get stuck. As @jenkm mentioned, an important part of any proposed change here would be how it affects other areas. So testing the other places that call WM_operator_props_dialog_popup to make sure they don’t get worse.

The space before the “okay” button seems like a generally good thing. But I have no idea why this change helps this one dialog, but doesn’t have similar affect on “image/new” and… crap… makes text editor “jump to” worse.

If you really don’t want to do this I can always pick it up later, but I’ve got a bit on my plate at the moment.

[phil.stopford]

Amusingly, a good number of these sites have a FIXME comment, noting the arbitrary sizing value :

  /* FIXME: hard-coded dimensions here are just arbitrary. */
  return WM_operator_props_dialog_popup(C, op, 250);

Existing values span quite a range:

[Harleya]

Although arbitrary, the value passed does get increased with user scale. When investigating keep in mind that we support lots of languages that might make some of our shorter strings really long (looking at you German language).

[phil.stopford]

The ‘jump to’ one really puzzles me. I don’t see why that’s ending up malformed in the way that it is. For the ‘new image’ dialog, the change makes no really noticeable effect. I’m trying to figure out how to summon the grease pencil dialogs to see what they end up looking like, but I’ve not used that feature before and am still digging.

[Harleya]

If it doesn’t become clear it might be best to just alter WM_operator_props_dialog_popup so that it takes those two enums as arguments. That way you can fix this one dialog while not changing the others. But it would still be better to understand what is going on so all of them can improve.

[phil.stopford]

I started toying with this and it struck me as a bit of a rabbit hole. I had in mind to create overloaded operators, so that the user could pass alignment and flags, with the conventional call just setting the defaults and calling forward.

From digging, this gets messy :

outliner_id_remap_invoke → WM_operator_props_dialog_popup → UI_popup_block_ex

UI_popup_block_ex takes function definitions that are called later :

UI_popup_block_ex(
  C, wm_block_dialog_create, wm_operator_ui_popup_ok, wm_operator_ui_popup_cancel, data, op )

as

void UI_popup_block_ex(bContext *C,
                   uiBlockCreateFunc func,
                   uiBlockHandleFunc popup_func,
                   uiBlockCancelFunc cancel_func,
                   void *arg,
                   wmOperator *op,
                   eButLabelAlign align,
                   short flag)
{
  wmWindow *window = CTX_wm_window(C);
  uiPopupBlockHandle *handle;

  handle = ui_popup_block_create(C, NULL, NULL, func, NULL, arg, NULL, align, flag);

ui_popup_block_create does this :

  /* store vars to refresh popup (RGN_REFRESH_UI) */
  handle->popup_create_vars.create_func = create_func;
  handle->popup_create_vars.handle_create_func = handle_create_func;
  handle->popup_create_vars.arg = arg;
  handle->popup_create_vars.arg_free = arg_free;
  handle->popup_create_vars.but = but;
  handle->popup_create_vars.butregion = but ? butregion : NULL;

So, I had thought about then adding properties to popup_create_vars to carry the alignment and flags (default or user specified).

From what I can tell, this rabbit hole may stop at ui_popup_block_refresh. It pulls out the create_func :

 const uiBlockCreateFunc create_func = handle->popup_create_vars.create_func;

and calls here :

  /* create ui block */
  if (create_func) {
    block = create_func(C, region, arg);
  }
  else {
    block = handle_create_func(C, handle, arg);
  }

So I think I can use this to pass in the align and flag values without breaking things.

However, I’m not sure if this is clean enough to pass review or there might be a preferred approach. What do you think?

In summary:

1. Create an overload of WM_operator_props_dialog_popup that adds align and flag parameters. The existing function would pass the existing default values :

   int WM_operator_props_dialog_popup(bContext *C, wmOperator *op, int width)
   {
     /* retain compatibility with previous behavior if caller has not provided alignment and flag */
     WM_operator_props_dialog_popup(
         C, op, width, UI_BUT_LABEL_ALIGN_SPLIT_COLUMN, UI_TEMPLATE_OP_PROPS_SHOW_TITLE);
   }
   

with the overload :

   int WM_operator_props_dialog_popup(bContext *C, wmOperator *op, int width, eButLabelAlign align, short flag)
   {
    wmOpPopUp *data = MEM_callocN(sizeof(wmOpPopUp), "WM_operator_props_dialog_popup");

    data->op = op;
    data->width = width * U.dpi_fac;
    /* Actual height depends on the content. */
    data->height = 0;
    data->free_op = true; /* if this runs and gets registered we may want not to free it */

    /* op is not executed until popup OK but is clicked */
    UI_popup_block_ex(
        C, wm_block_dialog_create, wm_operator_ui_popup_ok, wm_operator_ui_popup_cancel, data, op, align, flag);

    return OPERATOR_RUNNING_MODAL;
   }

2. Overload UI_popup_block_ex to allow the align and flag values to be passed. Overloading, to allow other call sites to remain unchanged, avoiding breakage. Similar to (1), the original function sets as passes on default values for align and flag, consistent with current behavior.

3. Overload ui_popup_block_create to allow align and flag values to be passed.

If this looks sane, the create_ui_block is where, I think, the next changes would be made, to pass the align and flag values forward.

[Harleya]

I’m away from my computer tonight but will take a look tomorrow. I thought this could be done with minimal complexity, but that might just be a bad guess.

Adding code and complexity adds to maintenance costs, so it’s hard to justify lots of change if it only makes this one dialog look nicer. But will take a look asap. But it is awesome that you are finding your way around, as there is always so much to improve.

[phil.stopford]

My thought behind the overloads was to keep compatibility with existing users, but as and when other dialogs need some adjustment, those sites could request what they need in terms of alignment and flags. I’m not certain that it’s ever going to be the case that one size fits all potential users. Overloading also seemed cleaner than a differently-named function, but I admit to uncertainty whether overloaded functions are accepted in blender.

I just couldn’t see a neater way to do this that wasn’t introducing arbitrary hard-coding somewhere else. I hope I didn’t overlook something obvious and simple along the way, though.

[phil.stopford]

Just curious whether you got a chance to look at this. No rush, really, from my side, though.

[jenkm]

I would recommend that you, before coding, first discuss the design, how it will work, and whether we want it, with Brecht/Julian/Campbell, just so that you don’t waste your time in vain.

[phil.stopford]

If that’s the approach to take, pinging @brecht, @julianeisel and @ideasman42 for advice re. Remap user dialog - where to go digging to make this resizable? - #26 by phil.stopford

Does that look sane / desirable? The aim was to give a minimally invasive change that would allow dialogs to be better presented if the need arose, whilst keeping default behavior otherwise. One particularly annoying dialog at the moment is the ‘replace user’, but I expect there might be others that would benefit from having some ability to override the default case.

[brecht]

If I understand correctly, the suggestion is to somehow add arguments to the general WM_operator_props_dialog_popup, to make it use a different type of layout than other popups. If so I think that’s bad both on a design and implementation level.

We should not deviate from the layout used by the properties editor and other popups, with the label on the same line. If make the dialog wider and reduce the proportion used by the label then the absolute space available for the label can remain as it is now.

Anyway, this particular ID remap one is obviously much too small. We can make that one wider already and solves most of that problem. I think it would be good to improve the size and layout of popups in general along with that, and I don’t think that should involve specific assumptions about operators and their properties.

[phil.stopford]

Given the awkward sequence in this system, it wasn’t obvious how to allow specific dialogs to be tweaked without affecting all of them. I noted in my comment how this sequence runs and how far away from the original caller you get before the layout behavior is set.
I could not see an alternative way to cleanly allow for certain dialogs to be treated differently without making a standalone function with its own hard-coded settings. After this realization, it seemed perhaps a more general solution was to have an overload that took arguments from the point where the dialog is called and passed them down to the eventual point where the dialog is constructed and the layout applied. The defaults are retained for the case where no arguments are supplied. This allows any affected dialog to request its own behavior without being rammed into hard-coded settings and/or requiring a number of dialog-specific call sequences.

This is also why I posted here originally. What does the more acceptable approach look like?

[pixelgrip]

I just wanted to say, that the remap function to me, is the fastest way to change a material on a model without nasty assign by hand or something similar.
The problem with this i found is,that after you remaped the old material ID to a new one,then the remap “function” change the old material (with the old ID) to non fake user.This is pretty annoing,even more if you have a material library of houndreds of materials,then you have to remember the old material name,scroll trough the material list, and set it to fake user as it was before.If you dont do that, then everytime you change the material ID the old material get lost after save the blendfile as you know.
Just saying that,i was not sure if that was a “bug report worth”,however it would be nice to have maybe a toogle in prereference maybe to set “remap keep fakeuser after swap material ID” or something.

happy blending

[jenkm]

Not sure what you want to achieve.

1 . The layout should remain the same (the label on the left) for consistency.

2 . You can easily increase the width and this is already enough to understand which ID is selected.

3 . In most cases, you need to select some ID from the list and there you see it in full.

4 . You can try to reduce the proportion used by the label, but it should still be sufficient for, like “Новий Ідентифікатор”. That is, you won’t win much. And it’s probably not that easy.

5 . Look, for example, at the Search Box, that’s where the size problem really is and it’s used everywhere and not just one menu.

[phil.stopford]

Increasing the width leads to an oversized panel because so much padding is introduced on the left. It would be more elegant to avoid that waste. It also leads to questioning (1) in your assertions. If the UI isn’t very efficient in certain cases, should that continue to be the case in the search of consistency, or would the user benefit from more screen-space for the stuff they need?

For (3), it’s sadly not all that true. If you had a few UntitledUntitled… entries in there, you cannot click to see which one is being used. The drop-down, when it opens, does not highlight or otherwise indicate the currently in-use one - there’s no hint at all. If you have moderate to large scenes, the inability to see what is going on wastes a lot of time.

In the meantime, I guess Brecht has something in mind here. I’m interested to learn what that looks like and to see it in action.

[phil.stopford]

Just to try and move this forward, I’ve submitted a differential here (⚙ D13604 Widen the remap user dialog in the outliner) that does the bare minimum of making the dialog, arbitrarily, wider. I don’t know what @brecht has in mind for the more efficient layout, so hope this can at least make it in to get some relief here in the interim.

[Vyach]

1. I agree that type should remain for prefiltering
2. Why this huge space is here? Is there any point to have it?
   

[phil.stopford]

That was what I was arguing to reduce, but @jenkm seemed quite opposed. I don’t see a way to deal with this in the existing code, however, and the strength of opposition deterred me from poking further at this; it seemed doomed to rejection.

[jenkm]

You can create a patch with the suggestion of any changes as you like. I’m just trying to explain to you why it will most likely be rejected.

1. The layout with the label & property on the same line should be kept for consistency with the rest of the UI. And Brecht has already told you exactly the same thing.

2. You can’t just change the proportions to a different but fixed value, you need to take into account various languages.

   

3. You could easily create a custom UI special for this pop-up, but I’m sure no one wants to maintain a separate piece of code just for one small dialog.

4. Not to mention that there is no problem with using this dialog. When you select an ID you see it in full. Then you see about 18 characters from the beginning, and the same number from the end, which is quite enough to check.

Besides, you have already looked at the code and seen that there is no simple solution here. There are many places in Blender where the text is truncated and it would be good to fix it. But it should be a more general solution rather than a tweak for a single case.