Skip to content
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

Table: calculating virtualRowHeight when the rows have a different height #173

Open
maximilianweidenauer opened this issue Sep 3, 2021 · 26 comments
Assignees
Milestone

Comments

@maximilianweidenauer
Copy link
Contributor

It should be possible that some rows have a different height than usual (when a row includes html, images or possibly more cases I can't think of right now).
When that is the case, we should probably take the div tag where all rows are located and calculate the average row height to use as virtualRowheight.
This could be a problem (performance) when the column-size changes and we need to re-adjust the virtualRowHeight.
This clearly needs some discussion

@maximilianweidenauer maximilianweidenauer added the needs discussion Needs input from project manager or discussion with community label Sep 3, 2021
@rjahn rjahn added this to the 1.7 milestone Dec 22, 2021
@rjahn rjahn modified the milestones: 1.6, 1.7 Mar 25, 2022
@rjahn rjahn modified the milestones: 1.9, 2.1 May 23, 2022
@rjahn rjahn modified the milestones: 2.2, 2.3 Feb 6, 2023
@rjahn
Copy link
Member

rjahn commented Feb 20, 2024

Current status?

@rjahn
Copy link
Member

rjahn commented Feb 28, 2024

???

@maximilianweidenauer
Copy link
Contributor Author

This Ticket is open since 2021 and I've never encountered an issue that would need this. So it is not needed

@rjahn
Copy link
Member

rjahn commented Feb 28, 2024

I guess it wasn't used because it didn't work or it just works?

Did you test this feature wit current implementation?

@maximilianweidenauer
Copy link
Contributor Author

I will test it.

@maximilianweidenauer
Copy link
Contributor Author

@mhandsteiner created a project for me with enough rows to enable virtualScrolling, then I've changed a field of a row to contain HTML text so that the row height would be different for these rows. I did that for every 5-10 rows until row 300.
While testing, the virtualscrolling seemed totally fine and there have been no issues!

2024-02-28.13-48-10.mp4

video added just in case

@rjahn
Copy link
Member

rjahn commented Feb 28, 2024

the toolbar looks wrong in your screenshot?

@gimmixAT
Copy link
Contributor

@rjahn is there an example project for this? As far as I can tell, Primereact works with a single item height in their virtual scroller logic so having differently sized row heights might produce some awkward results

@gimmixAT gimmixAT added awaiting Feedback and removed needs discussion Needs input from project manager or discussion with community labels Dec 4, 2024
@gimmixAT
Copy link
Contributor

gimmixAT commented Dec 4, 2024

I implemented a proof of concept that will look at the initial 100 rows and will cet the row height based on the maximum height. This seems to be the behavior in the java client @rjahn please correct me if I am wrong in this assumption.

@rjahn
Copy link
Member

rjahn commented Dec 5, 2024

In swing it isn't possible to have different row heights. In react it should be. This ticket shouldn't implement same height for all rows in react. I guess there was a problem with different row heights, but it shouldn't be with current primereact versions?

If different row heights are possible and don't cause problems with react, we should allow it.

Your PoC is a cool implementation and we should keep it, but not enabled by default. You could check a new table property like: sameRowHeight (boolean).
If this property is true, you could scan the available records, without fetching additional records and if user scrolls down and triggers fetching, you could update the height.

Do you already support rowHeight, maxRowHeight, minRowHeight in your calculation? because there are properties in WebTable.

If rowHeight is set -> fixed height but also check min and max
If minRowHeight is set -> minimum height
If maxRowHeight is set -> maximum height

So, if sameRowHeight is set -> calculate and if one of rowHeight, minRowHeight, maxRowHeight is set -> should work as well

@gimmixAT
Copy link
Contributor

gimmixAT commented Dec 9, 2024

I put the behavior behind and option as discussed.

Regarding rowHeight, minRowHeight and maxRowHeight - I don't think those are supported yet. The only way I can see how row heights are set right now is via the css theme. Are those props already set and sent in any of the feature screens?

@rjahn
Copy link
Member

rjahn commented Dec 10, 2024

Sorry, no usage right now in demo application. But it's easy to test.

User: features
Screen: SimpleContacts (com.sibvisions.apps.mobile.demo.screens.features.SimpleContactsWorkScreen in demo/src.client folder)

UITable table = new UITable();
table.setDataBook(rdbContacts);

// test code		
table.setMinRowHeight(5);

search table.setDataBook(rdbContact); and insert your test code below. Restart application server.

@rjahn
Copy link
Member

rjahn commented Dec 10, 2024

I made a test with row height and dynamic row height without setting sameRowHeight, looks good.

One problem: Server sends

<html>Hans<p/><p/>New Line</html>

and client renders:

<span>Leona <p></p><p>New Line</p></span>

If you want to test, simply svn update demo project and change

SimpleContactsWorkScreen -> replace:

rdbContacts.setName("contacts");
with:
rdbContacts.setName("contactsHtml");

Not sure if this is our problem or a browser problem?

@rjahn
Copy link
Member

rjahn commented Dec 10, 2024

I made tests with sameRowHeight and had a problem with row selection!

Test with features and Simple Contacts Screen.

Enable same row height - code is in comment:

/*
try
{
Reflective.call(table.getResource(), "setSameRowHeightEnabled", Boolean.TRUE);
}
catch (Exception e)
{
debug(e);
}
*/

Open the screen and press New (random) button multiple times. Sometimes, it's not possible to select another record by clicking on a different row.

@gimmixAT
Copy link
Contributor

the table should support minRowHeight & maxRowHeight now 30a3c5b

@rjahn
Copy link
Member

rjahn commented Dec 13, 2024

Did you fix row selection problem as well?

@gimmixAT
Copy link
Contributor

I wasn't able to reproduce that yet but I am still looking into it. I had to refactor the cell rendering a bit for the height measuring which caused some of the other table cell issues recently so It is very likely an issue of components not being recycled correctly.

@rjahn
Copy link
Member

rjahn commented Dec 13, 2024

It doesn't occur always, but often. Sometimes it's possible to select a record after some clicks, sometimes it's not.

Maybe this screencast helps:

select_record_table.mp4

and sometimes it happens, after you delete records, that the row-height is not the same for all records:

image

@rjahn
Copy link
Member

rjahn commented Dec 13, 2024

Did some tests: setRowHeight is always ignored.

I should explain it better:

default (sameRowHeight not set):

setRowHeight (rowHeight): if set, the rows should use this height (take care of min, max)
setMin/MaxRowHeight(min/maxRowHeight): take care of both values

sameRowHeight:

also take care of rowHeight (if set) and ignore the calculated row Height (better: don't calculate). Also check min and max if set.

no custom rowHeight set, but sameRowHeight:

calculate row height and take care of min/max (if set)

@gimmixAT
Copy link
Contributor

rowHeight from server is now supported as well

@gimmixAT
Copy link
Contributor

@rjahn regarding the rows not being selectable

It looks like selection requests are sent e.g. like this after creating multiple new rows and selecting the third row :

{
    "clientId": "SIx-a49db1f2-13c1-47a0-a39d-210d65c2f5fd",
    "componentId": "SimCon-QQ_T_contacts",
    "dataProvider": "JVxMobileDemo/SimCon-QQ/contacts#4",
    "filter": {
        "columnNames": [
            "ID"
        ],
        "values": [
            null
        ]
    },
    "selectedColumn": "SALU_SALUTATION",
    "rowNumber": 2
}

but the response is just an empty array

@rjahn
Copy link
Member

rjahn commented Dec 16, 2024

fixed row selection - was a server bug.

I will check rowHeight asap.

@rjahn
Copy link
Member

rjahn commented Dec 17, 2024

rowHeight is working if sameRowHeight is not set. In this case, all rows which are more than one line, will be bigger:

image

Also in case of sameRowHeight (true), the min/maxRowHeight is not working.
Just use Simple Contacts (DB) screen and play around with:

		try
		{
			com.sibvisions.util.Reflective.call(table.getResource(), "setSameRowHeightEnabled", Boolean.TRUE);
			
			rdbContacts.close();
			rdbContacts.setName("contactsHtml");
			rdbContacts.open();
		}
		catch (Exception e)
		{
			debug(e);
		}
		
table.setRowHeight(10);
table.setMinRowHeight(20);
table.setMaxRowHeight(25);

@gimmixAT
Copy link
Contributor

@rjahn in your example you also set a height via setRowHeight. If that was set I ignored the same height flag since it was implied that all rows would have that set height.
I enabled the same height flag in this case again but this results in the set height being ignored or only used as a base height for the initial layout

@rjahn
Copy link
Member

rjahn commented Jan 13, 2025

Hm.. not good, because if a developer sets a fixed row height, it should be exactly as set.
The developer could set the min row height to let it grow in height.

@gimmixAT
Copy link
Contributor

Ok I reverted that change again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants