-
Notifications
You must be signed in to change notification settings - Fork 48
Mapped list with index #83
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
Open
Symeon94
wants to merge
22
commits into
TomasMikula:master
Choose a base branch
from
Symeon94:mapped-list-with-index
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
927e975
Updating out deprecated blocks in graddle file
3cb558a
Updating the publication for 8+ version
Symeon94 833b137
Including the correct version of JavaFX in the build
Symeon94 f169edb
Mapped list with index
Symeon94 f5130a1
Removing item added by mistake
Symeon94 a927fd6
Adding tests for lazy evaluation and removing getSource() to use the …
Symeon94 d7d7393
Issue with outdated 'runtime' and missing JavaFX dependencies for rea…
Symeon94 8ca155a
Issue with outdated 'runtime' and missing JavaFX dependencies for rea…
Symeon94 6d0bc07
Javadoc fails building due to h2 title being used while parent is h3.…
Symeon94 120acab
Issue with build due to likely bug in the builder library. Upgrading …
Symeon94 411f081
merge
Symeon94 cf251ac
merge
Symeon94 a756475
merge
Symeon94 c5ccdad
merge
Symeon94 810958b
Using a secondary constructor instead of inheritance
Symeon94 95e7798
Removing generated gradle files
Symeon94 17e668d
Adding some comments to differentiate between the two methods
Symeon94 3764deb
Cosmetic cleanup
Symeon94 01a43d4
fixing issue when you are removing multiple items at the same time
Symeon94 431275d
Added unit tests
Symeon94 6fbb8a2
Forgot to remove a print
Symeon94 9255efa
Reversing to the original method to remove elements
Symeon94 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,3 +4,6 @@ build/ | |
| .project | ||
| .settings/ | ||
| .idea/ | ||
| gradle/ | ||
| gradlew | ||
| gradle.bat | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This doesn't look right.
It seems that you need a BiFunction version of
Lists.mappedView, and thenThere 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.
Thanks for taking the time to review.
I actually have tried that before you commented, but the problem is that the index provided by the mappedView is not the index of the removed item, it is the index in a new list.
If I have removed items at index 1 and 2, I want the apply to use the index 1 and 2. I guess the
iin your comment will be 0 and 1 in that case, which would result in index 1 and 3 ?(mod.getFrom() + i). Or said otherwise, we are mixing two differents unrelated types of index if we do that sum.So, I'm wondering about the correctness of that change. Maybe you have a scenario in mind where it would fail, if you describe it I can add it to the UT and make sure to fix it.
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.
Yes,
iwill range from 0 to 1. But thenmod.getFrom()will always return 1. So you get 1 and 2 as a result, which is as expected.Anyway, I haven't run it. A scenario where this should fail is if you remove a range of at least 2 elements.
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.
I'm a bit puzzled by the logic you are laying forth. I have actually already tested that case if you look at UT
testIndexedListwhich test the removal of index 1 and 2 (where you can see<index>-<len>below).It is my understanding that
mod.getFrom()returns the original index (as has been tested with the UT). I'm not sure where you are getting thatmod.getFrom()will always return 1?If I'm mistaken somewhere, please let me know (as you created the library, you definitly have a better understanding of the overall picture than I do).
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.
I could easily be mistaken, as I wrote the library 10+ years ago :)
However, in the code you quoted, you only remove a single element at a time (
strings.remove(2)). Try removing a range (e.g.strings.remove(2, 4)).For the same
mod,mod.getFrom()always returns the same number. That number is 1 if the change begins at index 1, as in your example.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.
There is no need to parse all the lines, I have already the code to identify the style change in only the visible portion of the code. The problem is that RichTextFX re-render and then applies the style after re-rendering, creating a visible delay between the two (which should be fixed with the index). I'll try to investigate when I have time on your proposal to play directly with the document prior to re-rendering and see if that can help.
(I'm not sure why you are proposing to parse the whole file each time, that is not something I'll do, as we reevaluate the style each time a user press a key, I cannot have a lag of 200ms on each key press).
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.
I'm not 😉
However, I did think you maintained an up-to-date version of all the styles.
Anyway, it seems to me that you are maintaining the styles somewhere on the side, while I'm saying the style should come through
EditableStyledDocument.If you do not want to maintain styles of the whole document, you should still be able to compute them lazily in
EditableStyledDocument#getParagraph(int)(e.g. for scrolling) plus eagerly for each edit.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.
For more precisions, I'm only keeping track of Python open/close group comments in my file. The rest of Python syntax can be computed based on the line content, which I do and only assess on changing the visible lines or on modifying a line. So each time a user changes a line, I use the information I saved about group comments to tell if the line is commented, and if not (or for the part that isn't) I recompute the style.
The styles are in the document, but it is hard for me to maintain that multiline comment based uniquely on the style class in the
EditableStyledDocument<Collection<String>, String, Collection<String>>.And, in any case, I don't think the style calculation is the right conversation here. The only problem is with a delay in the display.
Note that I'm not having delay in the inline style because I create it when opening my file. This creates the slow opening (although acceptable), which could be avoided if there wasn't any delay between rendering and styling. The issue arise as soon as I remove/add a group comment tag which must cause change potentially in the whole file. That's when you see the delay happen on scrolling (changing the visible lines)
Based on what you said in previous post, I thought you wanted me to try to create a custom
EditableStyledDocumentwhich would handle the logic of styling Python code. Did I understand correctly?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.
Correct me if I am getting this wrong, but the delay is there because the paragraphs were not styled correctly from the start, but only re-styled in reaction to a change in visible paragraphs (which is sent out only after the paragraphs are rendered).
Then all I'm trying to suggest is to get them styled correctly on the first render (not only after the visible notification).
Yes, a custom
EditableStyledDocumentthat would return correctly styled paragraphs on the first shot.Listening to visible paragraphs might still be useful for edits (i.e. on edit, do not restyle invisible paragraphs), but for scrolling just get the correct style from the get go.
Uh oh!
There was an error while loading. Please reload this page.
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.
Ok, thanks for confirming, when I'll get some time on my hand, I'll try to implement that custom document.
You are correct in your first statement (it's just that "not styled correctly" here, is just that they are not yet styled). Small correction, they are styled on 1. visibility change and 2. edition.