andlabs / libui

Simple and portable (but not inflexible) GUI library in C that uses the native GUI technologies of each platform it supports.
Other
10.73k stars 615 forks source link

Master issue for Table discussion #310

Open andlabs opened 6 years ago

andlabs commented 6 years ago

This thread exists to collect all the requests for Table and coordinate merging the feature back into master now that I'm ready to focus attention on it.

This thread includes by proxy the discussion in #159.

@bcampbell what's your current status on your Table Windows code, and what does it do currently?


Table will include Tree eventually, but not initially.

Requests from the Go side:

Other requests:

bcampbell commented 6 years ago

@andlabs: I've just brought my branch up-to-date: https://github.com/bcampbell/libui/tree/table (I've only tested it on Linux so far, but other than the uiFree/Alloc/Realloc rename it wasn't affected by the big utflib-and-attrstr merge).

All three platforms support:

The windows version supports only text columns (being based on the rather limited win32 commctl listview).

For me, the outstanding issues are:

  1. Some column-width management (not sure what API we'd want for this, if any. Maybe it should all be automatic - with random sampling of rows for big models...)
  2. a click event on column headers (eg to pick a column to sort by)
  3. the ability to sort by a particular column (with an indicator to show sort column and direction)

But I'm sure everyone would have their own wishlist here... For me, it's really useful already, as it stands.

Would opening a pull request against the main table branch be a good place to start?

andlabs commented 6 years ago

Sure! The other features you named can always be added later.

I also wonder what your stance on the "part" system currently is.

bcampbell commented 6 years ago

Regarding the part system, we probably need to step back and see what is possible.

The ideal case:

You can define custom layouts for use in cells. QML's listview delegate is the one I've used. (and see https://github.com/andlabs/libui/issues/159#issuecomment-335648135 for a screenshot example, from the windows explorer search) I'm not sure what the support for such custom layouts is on Cocoa or Gtk+, and it'd certainly take a load of work on the windows side. I think this is a good long-term aim.

But In the meantime, the existing parts API will certainly scratch a few people's itches. I'm inclined to think that uiTableAppendTextColumn() should be the only officially-blessed method. All the other parts functions can be left in for everyone who really needs them, marked with a big "here be dragons!" to make it clear that:

  1. they're not currently supported on windows
  2. they might be replaced in future with a more general API
bcampbell commented 6 years ago

(just to keep everything linked up...) I'm breaking up my uiTable hacking into smaller chunks. First one is now up, which just adds bare-bones windows table support:

https://github.com/andlabs/libui/pull/361

I'll move back on to selection handling etc when the dust settles.

andlabs commented 6 years ago

https://www.codeproject.com/Articles/35197/Undocumented-List-View-Features OH GOD DAMN IT MICROSOFT THIS IS EXACTLY WHAT WE NEEDED ALL ALONG

I am seriously tempted to say "screw it, this is too valuable to be ignored just because it's undocumented" and use it anyway. I'm not sure if I'll be able to use this directly because it seems to take over entire subitems and for the current parts system wouldn't allow that, but it's still miles better than the alternatives (change libui to LGPL for mCtrl or spend a lot of time rewriting my own table control again)! I wonder how this interacts with @bcampbell's suggestions about the parts system.

Now if only there was an equivalent that also supported tree views with multiple columns and full accessibility support...

andlabs commented 6 years ago
[22:23:21]  <+GerbilSoft>   fair warning: windows 7 has a *different* GUID for IListView
[22:23:55]  <+GerbilSoft>   https://github.com/GerbilSoft/rom-properties/blob/master/src/libwin32common/sdk/IListView.hpp
[22:23:56]  <+GerbilSoft>   https://github.com/GerbilSoft/rom-properties/blob/master/src/libwin32common/sdk/IOwnerDataCallback.hpp
[22:23:58]  <+GerbilSoft>   this has both
[22:24:42]  <+GerbilSoft>   the win7 one does work for win10 though
[22:24:45]  <+GerbilSoft>   and 8
[22:26:23]  <+GerbilSoft>   i have no idea why IListView is still undocumented
[22:26:30]  <+GerbilSoft>   it's effectively part of the ABI now that everyone uses it
[22:28:44]  <+GerbilSoft>   the win7 one adds a function, EnableAlphaShadow()
[22:28:51]  <+GerbilSoft>   ...right in the middle of other functions, thus shifting the vtable
[22:29:06]  <+andlabs>  yes, COM requires you to change the GUID in that case
[22:29:08]  <+GerbilSoft>   if it was at the end of the vtable, then they could've done IListView2 instead
[22:29:17]  <+GerbilSoft>   which inherits from IListView

(cc @gerbilsoft)

bcampbell commented 6 years ago

Wow. The ListView Control seems to be the control that just keeps on growing. I fully expect it to gain support for it's own process management, scheduling and device drivers...

Anyway.

At a very brief reading, it looks to me like their ListView can now support arbitrary control layouts, via the ISubItemCallback interface. I think GTK+ and Cocoa can do this too.

I'd guess that this 'undocumented' stuff is already in so much use that it may as well be an ABI. My suspicion is that it is, in fact, more-or-less documented, but only from the .NET (and maybe WinRT) point of view. Microsoft seem to be actively discouraging win32 development these days.

Lots more speculation follows:

There are a whole heap of existing ISubItemCallback controls available (hyperlinks, star ratings, calendar etc), where we could vastly enrich the libui table Parts system. But I don't think this is the ideal way to go - you end up with a two-tier layout system. What you really want is to be able to use a full libui layout to display a cell. I'm pretty sure there'll be a way of doing this, but I've no idea how the message pump and hwnd stuff would link in. I think you'd pass in a 'factory' struct, with functions set up for creating, populating and destroying (and editing) layouts, and detailing how the uiTableModel data maps to it. Behind the scenes, libui would wrap this factory up as a COM implementation for use in the IListView.

If IListView is anything like it's counterparts in other GUI toolkit (eg QtQuick/QML), it'll cache and reuse the control layouts, so you should require many more than the number of rows displayed.

So. How does this all affect the libui table API?

Firstly, 'appending' multiple parts to a column probably isn't the right model any more. You'd instead want to be able to set a single custom layout for each column. Obviously you'd have shortcuts for common cases - eg a plain text column.

I think a full custom-cell-layout system is a great goal to aim for, if it's possible. So I'd be inclined to hold back the parts API (or at least mark it 'turbulence likely') for now. Just implement text columns, which we'd want a shortcut for anyway, no matter what other fancy stuff we go with...

andlabs commented 6 years ago

All right. I'm still not taking any immediate action on the parts system; after I get this PR merged in, I want to go back through both this and your main criticism and see what to do about it.

The factory idea is something I've been thinking about when I do eventually get to headerbar-type controls, because on Windows if I use rebars and on Mac OS X with layout entirely I would be breaking the current rule that each control has a single handle made when created. It's something I'll need to think about for those.

As for this being undocumented, my only theory is that the interface is a mess that did change in COM-violating ways. It just makes me wonder what kept exposing this functionality some other way from beng considered worth exposing (such as via the -100 point system...).

andlabs commented 6 years ago

Also GTK+ uses cell renderers, not real widgets, and cell layouts that operate like GtkBox/uiBox. Cocoa has something like this, but most things now use view-based table views, where you can just shove a bunch of controls into a table cell. You do have to create the controls for every visible row in the table, but the system is smart and lets you reuse existing rows if they aren't in use. Interface Builder has some helpers for all this, but I forget what they are.

bcampbell commented 6 years ago

Just for reference: discussion about the Gtk+ cell layouts, and the (future) possibility to extend them to handle generic widget layouts: https://wiki.gnome.org/Projects/GTK+/GtkTreeView/Ideas#Renderer_objects

andlabs commented 6 years ago

Okay, tested accessibility of that demo program in the CodeProject link. Keyboard accessibility shows we can navigate through subitems, but not interact with them? Assistive technologies accessibility (aka automation) is even weirder: Inspect.exe acts as if the subitems aren't there unless we have already started editing one, but UI Spy does but we can't do anything with them? Definitely needs investigation.

I wonder if it's a 32-bit vs 64-bit issue, since the download included is 32-bit. Would need to build another 64-bit version with some changes I made locally reverted. Perhaps I could also test the 32-bit version of Inspect.exe and UI Spy, though I doubt they would affect things...

andlabs commented 6 years ago

Okay, going to analyze and respond to @bcampbell's stuff on table parts now, because I'm trying to redo the messy OS X table implementation and the parts system is inconsistent in ways that will affect the implementation changes I want to make, so maybe instead of making it consistent there's something better in these suggestions. They are

(I thought there was another page somewhere...) My response will be the next comment. Sorry for the delay in this!

bcampbell commented 6 years ago

My reading of the current state of full-blown arbitrary-widget-layouts-as-cell-contents support:

Cocoa: supported (I think)

Gtk: not currently supported, but there's talk. I'd guess it'll come in with Gtk+ 4 (due out later this year, but more realistically a year or two from mainstream, say). Currently there is some control over cell contents. Cells are handled with GtkCellRenderer and there are renderers for images, text and checkboxes. Maybe it'd be possible to derive a custom GtkCellRenderer which wraps an arbitrary widget layout?.

Windows: supported, via undocumented (but widely used?) ISubItemCallback interface. Requires that the widget layout can be wrapped up as a COM object, which sounds feasible. But probably not fun.

andlabs commented 6 years ago

See GtkCellLayout, GtkCellArea, and GtkCellAreaBox, the last of which the GTK+ uiTable already uses. For arbitrary layouts there's also GtkListBox, though that isn't multicolumn. The problem is that GtkListBox can get inefficient with very large listboxes, but GTK+ 4 should help fix that.

Still not sure just how flexible ISubItemCallback lets you go with regards to layout.


Response to concern about parts system:

The parts system was intended to be similar to the GtkCellRenderer API, actually: you could shove multiple different parts into a table column. I do now wonder how many people will want this arbitrary control, though. The parts system was also intended to act like a factory (since we can't just shove full widgets in a GtkTreeView my hand was forced in this regard anyway).

I always wanted to have a separate list view type for arbitrary layouts, since it always seemed like there was no real use for lists with arbitrary layouts to be columnar. But maybe there's an argument for having one widget provide both options, like what Windows Explorer does with its search results list view. Of course, GTK+ isn't as flexible with this; we'd have to basically write our own GtkCellArea subclass for anything more complicated than GtkCellAreaBox.

(OS X is also slightly annoying in that using Auto Layout for row heights is a massive kludge before 10.13.)

And then of course that leads to the possibility of only having one type of thing in each column, which is what you suggested instead of the parts system after I found IListView. My issue with that was that text, images, and checkboxes probably shouldn't be mutually exclusive like that, though maybe it makes more sense to limit things:

Whether a checkbox-image-text column is necessary is another story. We can get rid of expand and stretchy with this idea; that part is encoded in the column type. And editability would be a property of the column, though then the question of what is editable in a checkbox column, the checkboxes or the text.

One problem I saw when trying to rewrite the OS X uiTable was that attributes on text columns would need to be supplied somehow. I originally wanted to do a uiTableColumnPart type at that point, where you have a method like SetColorModelColumn() for things like text color. This would also allow Editable and Stretchy to be set as properties, but... eh. If we restrict ourselves to one type per column, that info would become part of the column instead.

Row layouts could then be done by providing a way to arrange columns.

Also I forget if we can even mix multiple table columns with custom row layouts on Mac OS X, or if we would need to have one NSTableColumn in that case.

Anyway sorry for the disorganized nature of this reply ^^;

andlabs commented 6 years ago

Also the view used in the Explorer search results screenshot seems to be the result of LVTVIF_EXTENDED, which was somewhat documented at one point and may not give us total control over the positioning of subitems, but still.

andlabs commented 6 years ago

How is this for an API?

// values of editable and clickable model columns for when you don't need granular control in a table model
#define uiTableModelColumnNeverEditable (-1)
#define uiTableModelColumnAlwaysEditable (-2)

// if pointers to this are NULL, nothing here is set; otherwise, columns you don't care about must be set to -1
typedef struct uiTableTextColumnOptionalParams uiTableTextColumnOptionalParams;

struct uiTableTextColumnOptionalParams {
    int ColorModelColumn;
};

_UI_EXTERN void uiTableAppendTextColumn(uiTable *t,
    const char *name,
    int textModelColumn,
    int textEditableModelColumn,
    uiTableTextColumnOptionalParams *params);
_UI_EXTERN void uiTableAppendImageColumn(uiTable *t,
    const char *name,
    int imageModelColumn);
_UI_EXTERN void uiTableAppendImageTextColumn(uiTable *t,
    const char *name,
    int imageModelColumn,
    int textModelColumn,
    int textEditableModelColumn,
    uiTableTextColumnOptionalParams *textParams);
_UI_EXTERN void uiTableAppendCheckboxColumn(uiTable *t,
    const char *name,
    int checkboxModelColumn,
    int checkboxEditableModelColumn);
_UI_EXTERN void uiTableAppendCheckboxTextColumn(uiTable *t,
    const char *name,
    int checkboxModelColumn,
    int checkboxEditableModelColumn,
    int textModelColumn,
    int textEditableModelColumn,
    uiTableTextColumnOptionalParams *textParams);
_UI_EXTERN void uiTableAppendProgressBarColumn(uiTable *t,
    const char *name,
    int progressModelColumn);
_UI_EXTERN void uiTableAppendButtonColumn(uiTable *t,
    const char *name,
    int buttonTextModelColumn,
    int buttonClickableModelColumn);
andlabs commented 6 years ago

Regardless of the above question, I am also going to change the way data is passed to and from the uiTableModel to something that operates similarly to uiAttribute:

typedef struct uiTableData uiTableData;

_UI_EXTERN void uiFreeTableData(uiTableData *d);

// TODO actually validate these
_UI_ENUM(uiTableDataType) {
    uiTableDataTypeString,
    uiTableDataTypeImage,
    uiTableDataTypeInt,
    uiTableDataTypeColor,
};

// TODO I don't like this name
_UI_EXTERN uiTableDataType uiTableDataGetType(const uiTableData *d);

_UI_EXTERN uiTableData *uiNewTableDataString(const char *str);
_UI_EXTERN const char *uiTableDataString(const uiTableData *d);

_UI_EXTERN uiTableData *uiNewTableDataImage(uiImage *img);
_UI_EXTERN uiImage *uiTableDataImage(const uiTableData *d);

_UI_EXTERN uiTableData *uiNewTableDataInt(int i);
_UI_EXTERN int uiTableDataInt(const uiTableData *d);

_UI_EXTERN uiTableData *uiNewTableDataColor(double r, double g, double b, double a);
_UI_EXTERN void uiTableDataColor(const uiTableData *d, double *r, double *g, double *b, double *a);

// ...

struct uiTableModelHandler {
    int (*NumColumns)(uiTableModelHandler *, uiTableModel *);
    uiTableDataType (*ColumnType)(uiTableModelHandler *, uiTableModel *, int);
    int (*NumRows)(uiTableModelHandler *, uiTableModel *);
    uiTableData *(*CellValue)(uiTableModelHandler *, uiTableModel *, int, int);
    void (*SetCellValue)(uiTableModelHandler *, uiTableModel *, int, int, const uiTableData *);
};

The final merge-back with master will have documentation added, but the documentation for uiAttribute should give you the gist. The only difference is that uiImage ownership is NOT transferred.

bcampbell commented 6 years ago

I think that API does simplify things, especially the common multi-Part cases (ie image + text). My only real unease with the current (old) Parts API is that it falls between the simple cases and the full-control-over-layout cases, so I think committing to one - as this new API does - is a good thing. It also leaves the door open for a (far future ;- ) uiTableAppendFullBlownCustomWidgetLayoutColumn() kind of thing without breaking anything.

Comments on the API itself:

Windows-specific stuff:

Looking at the bog-standard windows comctl listview, I'm pretty sure that most of this can be supported, with a couple of caveats:

If we want to embrace IListView and it's SetSubItemCallback we could support the whole API.

andlabs commented 6 years ago

do we really need per-row granularity to govern editable/clickable cells? ('yes' is a perfectly acceptable answer here, but off the top of my head I can't think of any cases I've seen in the wild where it wasn't uniform and per-column)

Not really, but it will make the implementation easier for the moment, since it means all API parameters are internally consistent. I can always change it later. It would also apply to, say, disable some checkboxes on a per-row basis.

the uiTableTextColumnOptionalParams pointer feels a little "un-libui". Would separate functions (eg uiTableBindColumnColor(uiTable* t, int column, int modelColorIndex) etc) be more in keeping with the style?

That would require supporting the case of being able to change model column bindings at any time, which OS X might not allow... or at least not make easy at all without sacrificing things like the reuse cache. There's probably a way that I'm missing.

It'd be very nice to have the ability to add and remove columns on the fly. Maybe 'insert' rather than 'append'? I'm thinking of UIs that allow the user to configure which columns are shown (eg windows explorer having columns for assorted file metadata).

The Append nomenclature I'm specifically using to add to the end of the list. There will also be InsertAt and Delete methods.

I think a hyperlink column would be wonderful :- ) (label from one model column, URL from another)

Sure, that can be done later. Added to top post.

only supports one checkbox state per row (and suspect checkbox is locked to first column)

Again, check the recent posts on The Old New Thing about listview checkboxes; we can fake it for subitems. There's also a checkbox subitem part for IListView.

can set colors, but only for whole table. But might be possible with some owner-draw hooks (hopefully without affecting accessibility).

Yes owner draw, yes without accessibility

no progressbar (but could use text instead (eg "42%"). (A proper progressbar might be possible via the undocumented IListView SetSubItemCallback and CLSID_CCustomDrawProgressControl).

We could also owner draw this. Combine with owner data for accessibility.

might only be able to have images in first column (unsure).

I'm pretty sure subitem images are supported...

bcampbell commented 6 years ago

Cool. All sounds good to me then!

andlabs commented 6 years ago

Might as well share current progress

Windows table view

Not sure how familiar this code will look to @bcampbell anymore, heh...

bcampbell commented 6 years ago

Hehe - I'm quite happy for all my code to be obliterated :- ) I just want cross-platform tables that'll support plain text columns, sorting, multiselect, clicking/doubleclicking and don't fall down on large (>1M row) data models. And golang bindings. Speaking of which, do you still want patches for multiselect and the like? I don't think it'll be too much work adapting it from my old table code (although I'd need a little help with some of the Cocoa stuff).

andlabs commented 6 years ago

Yes, after this branch is merged back into master (which means after I get all the functionality that's already there working =P ).

andlabs commented 6 years ago

Notes to self: the recent checkbox series:

And UI states, which I'm not sure are related, but I might as well throw them in anyway (they would go into a separate issue on libui too, so I need to do that too):

andlabs commented 6 years ago

Current progress:

Progress

The entire control is now custom-drawn. There is still no interaction or editing of values yet. I also need to handle focusing (both drawing the focus rect and subitem focus keyboard handling).

Indeterminate progressbars animate. I still need to figure out the GLib equivalent of the relevant C++ data structures so I can do the same in GTK+ (unless there's a better way).

Next up is buttons. For this I'm likely going on my own, so... :D

andlabs commented 6 years ago

i will not let winapi defeat me

Editing isn't perfect but it's a good way there. To really be perfect I'd have to use the undocumented subitem stuff =P

As for merging back into master, three things remain: drawing focus rects here, subitem navigation and activation with the keyboard, and one last round of cleanup.

andlabs commented 6 years ago

Bonus pic:

Bonus pic: it works unthemed Proper resolution

andlabs commented 6 years ago

Well that was a wash; LVM_SETSELECTEDCOLUMN does not actually select a column of a cell. It makes the entire column's background look slightly darker, as in Explorer under certain conditions I don't remember right now. Not sure what I'll do with subitem editing in the short term then; will decide later.

parro-it commented 6 years ago

There's something wrong with the bonus picture...

andlabs commented 6 years ago

What's wrong with it, other than GitHub resizing it? You'll need to actually click the link I just added to see the proper resolution :/

parro-it commented 6 years ago

Oh sorry, I missed the link... on GH I only see half window header, but on imgur it's ok...

msink commented 6 years ago

On Windows7, when text size set to 125% - edited text does not fit: image

And program crashes after exit.

msink commented 6 years ago

It was Kotlin version:

import libui.*

fun main(args: Array<String>) = appWindow(
    title = "Table",
    width = 800,
    height = 480
) {
    val model = TableModel {
        var row9text = "Part"
        var yellowRow = -1
        var checkStates = IntArray(15)
        val image0 = Image(16.0, 16.0) {
            add(image0_16x16, 16, 16, 64)
            add(image0_32x32, 32, 32, 128)
        }
        val image1 = Image(16.0, 16.0) {
            add(image1_16x16, 16, 16, 64)
            add(image1_32x32, 32, 32, 128)
        }

        numColumns { 9 }
        columnType { when (it) {
            3, 4 -> uiTableDataTypeColor
            5    -> uiTableDataTypeImage
            7, 8 -> uiTableDataTypeInt
            else -> uiTableDataTypeString
        }}
        numRows { 15 }
        getCellValue { row, col -> when (col) {
            0 -> TableDataString("Row $row")
            1 -> TableDataString("Part")
            2 -> TableDataString(if (row == 9) row9text else "Part")
            3 -> when (row) {
                     yellowRow -> TableDataColor(RGBA(1.0, 1.0, 0.0))
                     3         -> TableDataColor(RGBA(1.0, 0.0, 0.0))
                     11        -> TableDataColor(RGBA(0.0, 0.5, 1.0, 0.5))
                     else      -> null
                 }
            4 -> if ((row % 2) == 1) TableDataColor(RGBA(0.5, 0.0, 0.75)) else null
            5 -> if (row < 8) TableDataImage(image0) else TableDataImage(image1)
            6 -> TableDataString("Make Yellow")
            7 -> TableDataInt(checkStates[row])
            8 -> when (row) {
                 0    -> TableDataInt(0)
                 13   -> TableDataInt(100)
                 14   -> TableDataInt(-1)
                 else -> TableDataInt(50)
            }
            else -> null
        }}
        setCellValue { row, col, value -> when (col) {
            2 -> if (row == 9) row9text = value!!.string
            6 -> {
                val prevYellowRow = yellowRow
                yellowRow = row
                if (prevYellowRow != -1)
                    rowChanged(prevYellowRow)
                rowChanged(yellowRow)
            }
            7 -> checkStates[row] = value!!.int
        }}
    }

    add(widget = VerticalBox {
        padded = true
        add(stretchy = true, widget = Table(model, {
            textColumn("Column 1", 0, uiTableModelColumnNeverEditable)
            imageTextColumn("Column 2", 5, 1, uiTableModelColumnNeverEditable, 4)
            textColumn("Editable", 2, uiTableModelColumnAlwaysEditable)
            setRowBackgroundColorModelColumn(3)
            checkboxColumn("Checkboxes", 7, uiTableModelColumnAlwaysEditable)
            buttonColumn("Buttons", 6, uiTableModelColumnAlwaysEditable)
            progressBarColumn("Progress Bar", 8)
        }))
    })
}

But libui test.exe crashes too.

mischnic commented 6 years ago

And program crashes after exit.

On macOS the address sanitizer isn't happy either.


Also, the default column title text color on macOS is black: bildschirmfoto 2018-06-21 um 12 06 13 bildschirmfoto 2018-06-21 um 12 06 16

msink commented 6 years ago

Well,

$ gdb test.exe
GNU gdb (GDB) 8.1
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-w64-mingw32".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from test.exe...done.
(gdb) r
Starting program: F:\src\kotlin-libui\build\libui\out\test.exe
[New Thread 11772.0x2190]
[New Thread 11772.0x2d98]
warning: [libui] F:\src\kotlin-libui\libui\windows\alloc.cpp:25:uninitAlloc() You have a bug: Some data was leaked; either you left a uiControl lying around or there's a bug in libui itself. Leaked data:
0x2648e10 uiTableModel
0x50e5fe0 uiImage
0x50e7990 uiImage

Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
0x000007fefcf531f3 in KERNELBASE!DebugBreak ()
   from C:\Windows\system32\KernelBase.dll
andlabs commented 6 years ago

Yes, the crash is the test program not being fully updated yet.

The proper size of the edit on Windows is a TODO. The real listview has a lot more space at the right edge than I do, but I can't figure out the source of that space. (Also remember that libui still has DPI awareness turned off. I want to do the changes in this and windows-namespace-and-hresult-cleanup before I switch that on.)

Also I wonder if the icons in 125% mode should still be 16x16 or not.

Not sure about column headers on Mac; need to check both that code and Interface Builder again.

And yes, totally need to run asan on all this :D

msink commented 6 years ago

Trying to fix memory leak, ran into another bug - uiFreeImage not implemented in OSX version.

msink commented 6 years ago

Will there be handlers for onClick/onDoubleClick/onKey ? Some users do not want to edit in-place, prefer to open a Form instead.

andlabs commented 6 years ago

At some point in the future, yes.

szanni commented 6 years ago

Has any thought gone into sorting and filtering API wise?

GTK has GtkTreeModelSort and GtkTreeModelFilter, abstracting sorting and filtering from the underlying data model. It however seems to me that using these interfaces would just complicate things and make the implementation less flexible. On macOS the mechanism seem to operate directly on the data.

Ideas for sorting:

  1. Provide a callback uiTableColumnHeaderClicked(int i) and let the user sort the underlying data model and refresh the table via something like uiTableReloadData()
  2. Let the user provide a compare function for each column and sort internally uiTableRowCompare(int row1, int row2)
  3. Let the user provide a compare function for the entire table and sort internally uiTableRowCompare(int col, int row1, int row2)

The interface for 2 and 3 might seem a little odd at first, but quite often more than once column is needed to sort properly. Possibly even data that is not shown in the table?

Ideas for filtering

  1. User side filtering with a uiTableReloadData() function (no additional functionality needed in libui)
  2. A uiTableRowIsVisible(char *filter, int row) callback function.
  3. A uiTableFilter(char *filter) and uiTableRowIsVisible(int row) function

Again here for 2 and 3 I would go with a filter by row and not elements necessarily displayed in the table to allow for pre-processing. I would personally probably go for 3, as pre-processing (diacritics stripping, string splitting) is probably important for most implementations and seems inefficient to do on every isVisible() call.

Comments?

andlabs commented 6 years ago

I could merge this as is now, apart from documentation (which hopefully shouldn't take long to write). However, I can slightly optimize things on GTK+ if I split the model Int type into a ModelBool and ModelProgress types. I'm not sure if I should now, should later (breaking things), or not at all...

msink commented 6 years ago

If you ask for personal opinions - it should be merged, if test-page16 works on all platforms. On windows it works, on linux and mac I didnt check.

And one more very personal opinion - Table should be named TableView, and TableModel - just Table. Dont like that Model word...

andlabs commented 6 years ago

Tables are now in master.

andlabs commented 6 years ago

I forget, did I mark anything else as blocking Alpha 4? Because I might just go ahead and push that out now. I want to make some deeper changes next, which will make the Windows code (and hopefully all the other code too) more stable and responsive, and then I can do fun stuff like High DPI and text input =P

I'll get to the PRs I've been sitting on tomorrow.

msink commented 6 years ago

For me blocking is only #395

simplexidev commented 6 years ago

The only one (sort of) blocking for me is https://github.com/andlabs/libui/issues/364, and you have that one marked as blocking also.

andlabs commented 6 years ago

For CI I might change it to "will test live". It shouldn't hurt if I quickly remove bad files and manually upload fixed ones anyway. AppVeyor being slow can be dealt with separately.

For timer destruction I could just patch up uiUninit(), but again, you don't need to call that on program exit. The tester has it to ensure that libui cleans itself up properly.

msink commented 6 years ago

you don't need to call that on program exit. The tester has it to ensure that libui cleans itself up properly

But if it not called - some global system resources can be not disposed, at least on windows.

andlabs commented 6 years ago

Which global system resources stick around after the program exits? That doesn't seem right to me... Either way, fixing it now.

bcampbell commented 6 years ago

@andlabs: I meant to say: thanks for all your hard work on the new table code - It's much appreciated! It's all looking great so far.

It's been a little busy here, but I'm finally getting around to collecting the other table changes I had. (First up, #413 )