Implement Responsive Client-Side Autoscaling#84
Implement Responsive Client-Side Autoscaling#84Lightning11wins wants to merge 104 commits intomasterfrom
Conversation
…with an unknown type.
…yout, button, editbox, html, image, label, scrollpane, textbutton, & treeview.
Fixes components, dropdowns, and images. Renames flex variables to be more clear. Send parent_w and parent_h to client.
Add tab-height support to apos.c. Add design support to apos.c. Add IsDesign to the WgtrClientInfo struct. Fix spelling mistakes. Clean up.
Add an error message when cxsecVerifySymbol_n() fails. Improve an existing error message when htr_internal_WriteWgtrProperty() fails to write a property of an unknown type. Set Centrallix event listener to be explicitly non-passive, fixing a console error when later code assumes that calling preventDefault() is allowed.
Fix a bug in qprintf() that caused % and & characters inside conditional printing areas to always print, regardless of the condition. Improve documentation for qpfPrintf_va_internal() and qpf_grow_fn_t(). Clean up.
Add shortcut functions: ht_flex_x(), ht_flex_y(), ht_flex_w(), ht_flex_h(). Rename fl_scale_x (was total_fl_x). Rename fl_scale_y (was total_fl_y). Rename fl_scale_w (was total_fl_w). Rename fl_scale_h (was total_fl_h). Rename fl_parent_w (was parent_w). Rename fl_parent_h (was parent_h). Remove ht_flex_format_all and ht_flex_all(). Improve usage of new feature in previously updated widgets. Fix spelling mistakes. Clean up.
Clean up some apps.
Add Math.clamp() and Math.isBetween(). Add getParentSize(), getParentW(), and getParentH(). Refactor getRelativeX/Y/W/H() to call the new getRelative(). Refactor setRelativeX/Y/W/H() to call the new setRelative(). Add fast_setRelativeX/Y(). Add setResponsiveX/Y/W/H() using a new shared setResponsive(). Add responsiveness to moveTo() with the new functions. Fix style guide mistakes. Clean up.
Add support for more edge cases with undefined values to wgtrGetServerProperty(). Add the Log action (and docs). Add the ReloadPage action (and docs). Improve documentation for the Alert widget.
Add htr_action_point() to ht_render.js to handle the point action responsively. Refactor pane and window widgets to use the htr_action_point(). Improve documentation and code clarity for htutil_point(). Improve documentation for the point action.
Update al_reflow() to create responsive CSS using the setResponsive() functions. Improve documentation for al_childresize(). Optimize and clean up inefficient code.
Rewrite C & JS code to use responsive styling. Fix a bug that allowed multiple tabs to be selected at once. Fix spelling errors. Clean up.
Fix unresponsive area used on editbox widgets. Fix unresponsive area used on textarea widgets.
Add tab_w and tab_h parameters to tab_features.app to fix broken left and right tab locations. Remove unnecessary button widget from autoscale_test.app.
…uced by disabling clipping CSS by default).
Add dragging cursor styles for window widgets. Add cursor styles for the checkbox, radiobutton, scrollpane, treeview, and window widgets. Update existing cursor styles for the button, datetime, dropdown, imagebutton, tab, and textbutton widgets. Add the .dt_dropdown CSS class to make styling the datetime dropdown easier. Update table columns to use col-resize instead of move. Clean up cursor CSS styles to follow consistent code styling.
…atives. Rewrite getters and setters in the ClipObject polyfill to replace __defineGetter__() and __defineSetter__(). Rewrite getters in the datetime, dropdown, and radiobutton widgets to replace __defineGetter__(). Rewrite pg_area() constructor to replace __defineGetter__(). Define a base when parsing relative properties to fix edge cases. Remove unused debug setters in the table widget. Remove unused debug code.
…aster. Replace calls to fast_setRelativeX/Y() with calls to setRelativeX/Y().
Remove obsolete workaround code. Remove a completed todo. Improve a comment.
|
@greptileai Sorry, forgot to add the label. That should be fixed now, so can you do the review? |
Greptile SummaryThis PR implements responsive client-side autoscaling for Centrallix widgets. The implementation adds flexibility calculations to the server-side auto-positioning system (apos.c) and uses CSS Major changes:
Code quality observations:
Known issues (documented in PR):
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| centrallix/wgtr/apos.c | Core auto-positioning logic updated with flexibility calculations for responsive layout; includes comprehensive documentation comments and proper recursion checks |
| centrallix/htmlgen/ht_render.c | Updated HTML rendering with modern standards (HTML5, UTF-8), responsive dimension support, and additional data type handling; includes several TODOs for future cleanup |
| centrallix/include/ht_render.h | Added responsive dimension macros and helper functions for flex calculations; clean interface additions |
| centrallix-os/sys/js/ht_geom_dom1html.js | Client-side responsive geometry implementation with setResponsive functions; moveTo bug was already fixed |
| centrallix/htmlgen/htdrv_tab.c | Tab widget updated for responsive layout with flexible positioning; contains memory leak TODO at line 351 |
| centrallix/htmlgen/htdrv_page.c | Page widget updated with responsive support and modern HTML meta tags; includes TODO for global variable handling |
| centrallix/htmlgen/htdrv_table.c | Table widget updated for responsive dimensions; contains TODO about scrollbar visibility logic at line 178 |
| centrallix-os/sys/js/htdrv_scrollpane.js | Complete rewrite of scrollpane widget using modern JavaScript (ES6+) with improved event handling and resize observer |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Server: wgtrVerify in wgtr.c] --> B[aposAutoPositionWidgetTree]
B --> C[aposPrepareTree: Patch unspecified heights]
B --> D[aposBuildGrid: Create layout grid]
D --> D1[aposAddLinesToGrid: Draw edge lines]
D --> D2[aposAddSectionsToGrid: Divide into rows/cols]
B --> E[aposSetFlexibilities: Calculate flex values]
E --> E1[Set fl_x, fl_y, fl_w, fl_h]
E --> E2[Set fl_scale_x/y/w/h]
B --> F[aposSetLimits: Honor min/max sizes]
B --> G[aposAutoPositionContainers: Position widgets]
G --> H[Server Rendering: htdrv_*.c files]
H --> H1[htrFormatElement with ht_flex macros]
H1 --> H2[Generate CSS calc with responsive formula]
H2 --> I[Client: HTML/CSS sent to browser]
I --> J[Client JS: setResponsive functions]
J --> J1[setResponsiveX/Y/W/H apply flex values]
J --> K[Browser renders responsive layout]
K --> L[ResizeObserver detects window resize]
L --> M[CSS calc updates dimensions automatically]
Last reviewed commit: 88e5969
|
@greptileai I've fixed the indicated issues. Please conduct a thorough review and let me know if there is anything else I should fix. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| char* config_buf = nmSysMalloc(bufsiz); | ||
| if (config_buf == NULL) | ||
| { | ||
| mssError(1, "HTTAB", "%s: nmSysMalloc(%d) failed.", name, bufsiz); | ||
| return -1; | ||
| } | ||
| snprintf( | ||
| memset(config_buf, 0, bufsiz), bufsiz, | ||
| "{ tlocs: { Top:%d, Bottom:%d, Left:%d, Right:%d, None:%d } }", | ||
| Top, Bottom, Left, Right, None | ||
| ); | ||
| htrAddScriptGlobal(s, "tc_config", config_buf, HTR_F_VALUEALLOC); | ||
| /*** TODO: Greg - config_buf is definitely leaked because I can't | ||
| *** figure out how long it needs to remain in scope. | ||
| ***/ |
There was a problem hiding this comment.
Memory leak: config_buf allocated with nmSysMalloc() but never freed. The buffer is passed to htrAddScriptGlobal() with HTR_F_VALUEALLOC flag, but it's unclear if that function takes ownership.
Check whether htrAddScriptGlobal() with HTR_F_VALUEALLOC frees the buffer, and if not, add proper cleanup.
There was a problem hiding this comment.
That function does not take ownership, however, it's also not possible for any function in htdrv_tab.c to know when this data is safe to free. I'll need to discuss this problem with @gbeeley because there are a few possible solutions, and none of them are very good.
For now, ignore this one specific problem, as it has a TODO that clearly indicates it, so we'll know to circle back and fix it later.
There was a problem hiding this comment.
The function does take ownership of strings marked with HTR_F_NAMEALLOC or HTR_F_VALUEALLOC. Various functions that take flags indicating allocation will take ownership of the allocated item (such a flag is not useful in any other way).
However, the freeing of these values is done using nmFree(), not nmSysFree(). That particular imbalance will essentially cause a memory leak.
In reviewing the code, I also see an issue in that htrAddScriptGlobal() under the hood assumes that the allocated item has a memory block length == strlen()+1 for the purpose of the resulting nmFree(), which is not a reliable way of handling that. A more reliable way would be to switch htrAddScriptGlobal(), and maybe other similar functions, to expect nmSysMalloc() to be used for names and values marked as ALLOC, instead of nmMalloc().
For memory allocation lifetime purposes, ALLOC flags (and actually dynamically allocating the memory) should be generally used for all non constant values. For constant values, omit ALLOC flags of course. Other situations may also exist where the htrAddScriptGlobal() caller can otherwise guarantee the lifetime of the memory until after page generation happens, in which case ALLOC flags need not be used.
|
@greptileai Can you find the cause for the first known issue? The one where table row details sometimes don't appear, even when the row is selected? |
|
This PR has been replaced by #85. |
2 participants
This PR has been replaced by #85.
I'm creating the PR for this project to begin review, starting with Greptile before moving on to other human reviewers.This PR resolves #25.This PR includes severalTODOs:3TODOs for Israel to handle after Update Duplicate Detection #77 is merged intomaster.4TODOs for Greg (although other reviewers are welcome to handle these if they have the requisite knowledge), in the following files:ht_render.c,htdrv_page.c,htdrv_tab.c, andhtdrv_table.c.There are several known issues with this code:Table row details sometimes don't appear.e.g. Broken left menus on the Kardia home page.Code that breaks responsive layout.widget/pane: resize action (testapps/kardia/modules/payroll/pay_form.app)widget/image: offset & scale actions (not used anywhere)widget/objcanvas: add_osrc_object()ht_get_parent_w/h__INTERNAL()&wgtrGetContainerWidth/Height()should be merged.They appear to give almost identical results, except when checking a top-level widget. In this case,ht_get_parent_h__INTERNAL()(mine) gives a height 2px taller.Flexibility calculations that depend on page size cause breakage since the client assumes widget flexibility scale is constant.This would be very hard to fix! The best solution I can think of so far is to rewrite the entire layout system to convert px -> % on the server, then do all layout client-side. However, I'm not sure this system could handle flexibility values at all.