Added line numbers in file detail view with ids for future styling. by k10blogger · Pull Request #157 · websvnphp/websvn

k10blogger · 2022-02-05T19:29:56Z

This is a minor enhancement. Previously the view looked like the following:

There were no line numbers in the file details view.

The change in the PR adds the line number and provides a basic default styling looks like the following:

The change uses the in-built line number system provided by geshi. So viewable only if geshi is used. Same can be done for enscript but I have to read up on it.
Further added an argument highlight to the applyGeshi function to identify file and line highlighting.
The line highlighting number is taken care internally it is only during file highlighting that the line numbers are missing.

…ighting.

michael-o · 2022-02-05T19:35:31Z

This is fantastatic, I will happily review this next week.

michael-o

I see two problems:

Anchors aren't clickable, means hard to share
Generated ID unusable since it changes constantly, see https://www.wikiparques.com/extensions/SyntaxHighlight_GeSHi/geshi/docs/geshi-doc.html#adding-ids-to-each-line. I guess we need $geshi->set_overall_id('...') as well?

…_number}

k10blogger · 2022-02-06T17:21:30Z

I was wondering how to change the overall ids. Thanks for pointing that out.
Yes anchors are not clickable but we can jump to anchors as follows(example link):

http://localhost/websvn/filedetails.php?repname=Test+Group+1.Common+Model&path=%2Ftrunk%2FCFiles%2Fasm08.c#line-135

Using line-{line_number}

michael-o · 2022-02-06T17:31:22Z

 		if ($highlight == 'file') {
-			$this->geshi->enable_line_numbers(GESHI_FANCY_LINE_NUMBERS);
+			$this->geshi->enable_line_numbers(GESHI_FANCY_LINE_NUMBERS,1);
+			$this->geshi->set_overall_id('line');


Attention, this does not only apply to the line, but to the entire geshi rendering.

Yes this applies to all lines...

line- is too naive. It will apply to all elements of the Geshi rending, not just lines. Lets make it geshi just like the set_overall_class()

Are you sure?? making geshi would mean if we have to get a url for say line number 110 we have to use anchor as #geshi-110. I am thinking line would be more intuitive. like #line-110 knowing we are trying to jump to line 110. or should we use l instead, like done in blame.php ??
Here is the url in case of blame http://localhost/websvn/blame.php?repname=Test+Group+1.Common+Model&path=%2Ftrunk%2FCFiles%2Fasm08.c&rev=23#l110

Yes, I am sure. Look into geshi.php and usage of overall_id. It does not only apply to numbers. It is all or nothing. As far I can see l110 is not possible because the template is {id}-{number}. The hyphen remains.

Okay... Changed to geshi.

michael-o · 2022-02-06T17:42:51Z

 		$this->geshi->set_tab_width($this->repConfig->getExpandTabsBy());

+		if ($highlight == 'file') {
+			$this->geshi->enable_line_numbers(GESHI_FANCY_LINE_NUMBERS,1);


The 1 doesn't add any benefit. It will just make every line bold.

By default fancy line numbers are normal line numbers. Adding 1 as an argument makes them "fancy" or "bold". Initially i had not passed any arguments as i was hoping maybe in future we can somehow pass the line number in the url here. So only those line numbers would be marked as bold. Link in case i pass #line-135 then only line 135 will be marked rest will be normal.

By default fancy line numbers are normal line numbers. Adding 1 as an argument makes them "fancy" or "bold". Initially i had not passed any arguments as i was hoping maybe in future we can somehow pass the line number in the url here. So only those line numbers would be marked as bold. Link in case i pass #line-135 then only line 135 will be marked rest will be normal.

This is not what I see. The number means in what step numbers will be bold. I don't see a reavery to make every number bold. There is no JS magic behind.

ahh okay. Anyways That is a topic for another PR. Completely dropped the 2nd argument.

michael-o · 2022-02-06T17:46:52Z

 		if ($highlight == 'file') {
-			$this->geshi->enable_line_numbers(GESHI_FANCY_LINE_NUMBERS);
+			$this->geshi->enable_line_numbers(GESHI_FANCY_LINE_NUMBERS,1);
+			$this->geshi->set_overall_id('line');


line- is too naive. It will apply to all elements of the Geshi rending, not just lines. Lets make it geshi just like the set_overall_class()

michael-o · 2022-02-06T17:47:12Z

+		if ($highlight == 'file') {
+			$this->geshi->enable_line_numbers(GESHI_FANCY_LINE_NUMBERS,1);
+			$this->geshi->set_overall_id('line');
+			$this->geshi->enable_ids(true);


true is default value, you can drop it and use zero args.

Reanmed the id to match overall class 'geshi'

michael-o · 2022-02-06T18:09:40Z

-			$this->geshi->enable_line_numbers(GESHI_FANCY_LINE_NUMBERS,1);
-			$this->geshi->set_overall_id('line');
+			$this->geshi->enable_line_numbers(GESHI_FANCY_LINE_NUMBERS);
+			$this->geshi->set_overall_id('geshi');


I don't see a better way without fixing geshi first. The issue that Geshi can be used multiple times in a PHP script, mean you would produce clashing its. That is why you need an overall id.
Bascially, they need to introduce a line number override and we make sure that only a single instance of geshi is used per svnlook. Which is.

michael-o

Please squash

…yling. (#157)" This reverts commit 0bcd697.

k10blogger · 2022-02-07T03:15:23Z

@michael-o Unfortunately i did a squash in web view which merged all the commits and changes to the base branch. Please have a look here. If needed i can revert and add changes again.

michael-o · 2022-02-07T07:02:58Z

@michael-o Unfortunately i did a squash in web view which merged all the commits and changes to the base branch. Please have a look here. If needed i can revert and add changes again.

Not necessary, take it as lessons learned.

k10blogger added 2 commits February 6, 2022 00:27

Added line numbers in file detail view with ids for future styling.

ba4dbf5

Added $highlight parameter as to not disturb the other types of highl…

0e8e80e

…ighting.

k10blogger requested review from ams-tschoening and michael-o February 5, 2022 19:29

ams-tschoening suggested changes Feb 5, 2022

View reviewed changes

Comment thread include/svnlook.php

Added new line before if statements for readability.

f232f92

ams-tschoening approved these changes Feb 6, 2022

View reviewed changes

michael-o requested changes Feb 6, 2022

View reviewed changes

Added overall id line. So line anchor can be referenced as line-{line…

73c9758

…_number}

michael-o reviewed Feb 6, 2022

View reviewed changes

michael-o requested changes Feb 6, 2022

View reviewed changes

Dropped the second argument for fancy line numbers.

2a607be

Reanmed the id to match overall class 'geshi'

michael-o reviewed Feb 6, 2022

View reviewed changes

k10blogger merged commit 0bcd697 into websvnphp:master Feb 7, 2022

k10blogger added a commit that referenced this pull request Feb 7, 2022

Revert "Added line numbers in file detail view with ids for future st…

d2e679e

…yling. (#157)" This reverts commit 0bcd697.

Conversation

k10blogger commented Feb 5, 2022

Uh oh!

michael-o commented Feb 5, 2022

Uh oh!

Uh oh!

michael-o left a comment

Choose a reason for hiding this comment

Uh oh!

k10blogger commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-o Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-o left a comment

Choose a reason for hiding this comment

Uh oh!

k10blogger commented Feb 7, 2022

Uh oh!

michael-o commented Feb 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

k10blogger commented Feb 6, 2022 •

edited

Loading

michael-o Feb 6, 2022 •

edited

Loading