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

Add line numbers support #2062

Merged
merged 20 commits into from
Aug 14, 2023
Merged

Conversation

guanglinn
Copy link
Contributor

  1. Add line numbers support for Edit mode and code blocks in View mode;
  2. Update Prism from v1.17.1 to v1.29.0.

@gsantner gsantner linked an issue Jun 5, 2023 that may be closed by this pull request
4 tasks
@@ -490,6 +498,11 @@ public boolean getDocumentHighlightState(final String path, final CharSequence c
return getBool(PREF_PREFIX_HIGHLIGHT_STATE + path, lengthOk && isHighlightingEnabled());
}

public boolean getEditorLineNumbersState(final CharSequence chars) {
final boolean lengthOk = chars != null && chars.length() < (_isDeviceGoodHardware ? 100000 : 35000);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you used some performance related values here... How well is performance overall, does it greatly impact i.e. scrolling or loading speed? Last time I visited this, it made everything horribly slow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't do enough testing. It is important but I'm not good at testing, could you have a test for it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a moderate to big textfile and scroll a bit. You will immediately notice when it's slow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only draw line numbers for the visible lines +- 2 pages.

The highlighter class does this. We can look at it / reuse some code imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, the algorithm or method for displaying line numbers is still worth optimizing. Second, the editor's loading strategy for large files may also need to be optimized, such as just loading nearby 1000 lines at a time, which can gracefully load large files. I could try to optimize it if I'm free.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guang-lin Note that Incrementally loading files with Android can be a pain as the underlying file system is unreliable. Our current system (trying to save multiple times etc) is hacky but prevents data loss 99.99% of the time.

Copy link
Owner

@gsantner gsantner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your help! I looked through and added my thoughts

Copy link
Contributor

@k3b k3b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view the code looks good

@k3b
Copy link
Contributor

k3b commented Jun 7, 2023

Note: Today i go on holyday for 3 weeks where i have no computer so i cannot do any code reviews in this time

@guanglinn
Copy link
Contributor Author

Note: Today i go on holyday for 3 weeks where i have no computer so i cannot do any code reviews in this time

Fine, have a good holiday!

@k3b
Copy link
Contributor

k3b commented Jun 26, 2023

From my point of view every thing else looks good

@harshad1
Copy link
Collaborator

harshad1 commented Jul 8, 2023

What is the status here? Any way I can help move this along?

for (int i = 1; i < count; i++) {
if (text.charAt(layout.getLineStart(i) - 1) == '\n') {
number++;
y = layout.getLineTop(i);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this look in todo.txt where we add extra padding to the top of the line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the screenshot, not bad.

Screenshot_20230708_161105_net.gsantner.markor_test.jpg

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the todo.txt improvement I pushed
screenshot-2023-07-09__20-29-36

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks better 😊

@guanglinn
Copy link
Contributor Author

What is the status here? Any way I can help move this along?

I think the status is stable. It can be better improved, if there are better ideas in the future.

@gsantner
Copy link
Owner

gsantner commented Jul 8, 2023

Hello, I try to find some time on this (hot) weekend to review again and hopefully merge this week.

Regarding todo.txt, would it be possible to add a offset at the number so it better fits (vertical center/middle) to the line? That offset can be conditionally activated by checking the format

@harshad1
Copy link
Collaborator

harshad1 commented Jul 8, 2023

Hello, I try to find some time on this (hot) weekend

You're in Austria Right? I was in Germany a few weeks ago and it was quite warm :)

Regarding todo.txt, would it be possible to add a offset

I think we can look at Font Metrics and align the bottom of the number to the line's baseline. This will be uniform in all cases and will not require special a case for Todo.

@gsantner
Copy link
Owner

gsantner commented Jul 9, 2023

Yes!

Hm, I think at the end it would be the best solution if the text is vertical-centered to the (full) line. Especially also when it's multiline (regarding line wrapping)

@harshad1
Copy link
Collaborator

harshad1 commented Jul 9, 2023

Hm, I think at the end it would be the best solution if the text is vertical-centered to the (full) line

I have to disagree. Vim and VsCode both align the bottom of the0 number to bottom of the first letter in the line. I think this is now it should go.

@guanglinn
Copy link
Contributor Author

I have aligned line numbers text to the right.

Screenshot_20230715_074012_net.gsantner.markor_test.jpg

@gsantner
Copy link
Owner

gsantner commented Aug 5, 2023

@guang-lin
Thanks for additional improvements. Do you think it's ready to merge now?

@guanglinn
Copy link
Contributor Author

@guang-lin Thanks for additional improvements. Do you think it's ready to merge now?

I think yes. It's my pleasure to contribute to this project. 😊

protected void onDraw(Canvas canvas) {
super.onDraw(canvas);
// If line numbers can be drawn
if (_nuEnabled && _maxLineNumber < (AppSettings._isDeviceGoodHardware ? 5000 : 3000)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As line numbers have to be enabled manually, perhaps we should just issue a toast warning instead of hard blocking on isDeviceGoodHardware

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also just try to make line numbers dynamic (draw them only for visible lines). That will avoid this completely. Lets remove this test for the line count at first and then I can help with the dynamic line numbers...

@guanglinn
Copy link
Contributor Author

guanglinn commented Aug 8, 2023

Now support iterating from the first visible line rather than the second line when the view is not scrolling, even at around 30,000th line.

@gsantner gsantner changed the base branch from master to linenumbers August 14, 2023 22:40
@gsantner
Copy link
Owner

Changed the branch to linenumbers branch. Merging this PR and improvements can be continued via additional PRs to linenumbers branch. Then we can merge it to master when you think it's fine.

Thanks for the contribution!

@gsantner gsantner merged commit 667b5d7 into gsantner:linenumbers Aug 14, 2023
1 check passed
@guanglinn guanglinn deleted the line_numbers_support branch December 25, 2023 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants