-
Notifications
You must be signed in to change notification settings - Fork 38
Added refreshCells method #135
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
base: master
Are you sure you want to change the base?
Conversation
The changes seem fine, but I have to say that I don't understand what situation you are tackling with this one. The index is needed only when we create them item isn't it ? And if that is not the case, normally the update on the LiveList should change the item itself (as shown by the UT for the LiveList). Could you give a case where this would be broken? |
I'm thinking of the following use case in RichTextFX when using Say you have a code editor and for comment blocks you have one colour and for JavaDoc you have another. When the developer changes the first line of the comment block to a JavaDoc then that line will automatically get restyled as it has changed. However the following lines of the comment will not be restyled unless a change of some sort occurs on them. There are two options here:
In both cases above the visible only styler will be invoked and which you use is dependent on your circumstances. |
Ok. My only comment (but maybe that will come with your changes in RichTextFX) is that maybe we should test via a UT that it does refresh the paragraph with the appropriate index number (for instance, instead of modifying line Aside from this little proposal, your in better position than me to assess the validity of your change (I confess that I have not touch my style code for a few weeks, I'm waiting for your change to fix the style update issue). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it's much better now (I still don't see UT though).
I think the exception if fromItem >= toItem
might be a bit radical (but feel free to ignore my comment).
*/ | ||
public void refreshCells(int fromItem, int toItem) { | ||
int start = Math.min( fromItem, toItem ); | ||
int end = Math.max( fromItem, toItem ); | ||
if (fromItem >= toItem) throw new IllegalArgumentException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is radical, there is no harm simply not doing anything in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a small note, in any case the fromItem < toItem
will cover that case, so you could simply remove your exception and it would work fine.
Added another UT, if you think there are other cases that should be checked please let me know. |
Thanks for taking the time to do that. I listed them previously:
|
This adds the ability to refresh a Cell without updating it's item. This is needed in situations where the context the item is in changes the way it is displayed, without the item itself changing.