Code region remains highlighted in the editor when unfolded using command Unfold All Lines

[tokengamedev]

Godot version

4.2.beta.3

System information

Windows 11

Issue description

Code region when unfolded using command Edit -> folding -> Unfold All Lines. keeps the # region line highlighted even though the region is no more folded.

Steps to reproduce

• Create a region of code using # region ..#endregion comments
• Collapse/Fold the region
• Notice the #region line is highlighted
• Click Unfold All Lines under script menu Edit -> Folding -> Unfold All Lines Expected - region will be expanded/unfolded and the highlight on the line will be gone. Actual - region is expanded, but highlight on the line is still there

Minimal reproduction project

N.A

[Nirhar]

Hey, I'm new to Godot and would like to start contributing with this issue. Can it be assigned to me?

[kitbdev]

@Nirhar You don't need to be assigned on github, you're welcome to tackle the issue. See this for more details.

[Heliotrop3]

When reproducing, the only part that remains highlighted after unfolding all lines is the line containing "# Region" which seems like normal line highlighting behavior. If any other line is selected after folding the lines but before unfolding then nothing related to the code region appears to be highlighted.

[Nirhar]

So here is my analysis of this issue: My understanding is that when one clicks on the Unfold All Lines edit option, the following handler is called: https://github.com/godotengine/godot/blob/b662d232a270cb6827e384ac778bf3a2f35794c7/editor/plugins/text_editor.cpp#L413-L415

However, to me it seems like CodeEdit::unfold_all_lines() merely just calls _unhide_all_lines(), instead of calling unfold_line on each line: https://github.com/godotengine/godot/blob/b662d232a270cb6827e384ac778bf3a2f35794c7/scene/gui/code_edit.cpp#L1736-L1738

I think it is essential that we call the unfold_line as this contains the code to reset the background color as well: https://github.com/godotengine/godot/blob/b662d232a270cb6827e384ac778bf3a2f35794c7/scene/gui/code_edit.cpp#L1703-L1727

In short my solution would be to call unfold_line on every line for unfold_all_lines(similar to fold_all_lines). Would that be an acceptable solution?

[kitbdev]

Either that or run just the reset highlighting bit on every line.

[tokengamedev]

I think calling the unfold in a loop is the right idea. It reuses existing logic, which makes sense. DRY principle. Although, it is the correct approach, there are couple of glaring issues I see in the unfold code to be used in loop, which may have to be addressed, or may create additional performance issues.

1. The call to queue_redraw at the bottom. Calling it in a loop may have some issues in UI. I am not sure about it.
2. The logic for unhighlighting the code needs to be called once, but it is inside a loop. Although it has a condition, but it can be fixed by just checking on the fold start

for (int i = fold_start + 1; i < get_line_count(); i++) { 
 		if (!_is_line_hidden(i)) { 
 			break; 
 		} 
 		_set_line_as_hidden(i, false); 
 		if (is_line_code_region_start(i - 1)) { 
 			set_line_background_color(i - 1, Color(0.0, 0.0, 0.0, 0.0)); 
 		} 
 	} 

could have been written as

for (int i = fold_start + 1; i < get_line_count(); i++) { 
 		if (!_is_line_hidden(i)) { 
 			break; 
 		} 
 		_set_line_as_hidden(i, false); 
 	
 	}
       // Did a lot of hardwork in previous loop to find the start of folding
	if (is_line_code_region_start(fold_start)) { 
 		set_line_background_color(fold_start, Color(0.0, 0.0, 0.0, 0.0)); 
 	}  

Additionally, the code in unfold_line can be optimized further, but is beyond the scope of the PR.

My idea will put the loop in a private method _unfold_line(fold_start) then the solution would look like

void CodeEdit::unfold_all_lines() { 
 	//_unhide_all_lines(); 
      bool has_updated = false
       for (int idx = 0;  idx < get_line_count(); idx++) { 
 		if (is_line_folded(idx)) { 
                        has_updated = true
                        _unfold_line(idx)
                       
                        if (is_line_code_region_start(idx)) { 
 		         set_line_background_color(idx, Color(0.0, 0.0, 0.0, 0.0)); 
 	              }  
                }
          if (has_updated)
              queue_redraw()
 } 

pardon the code above for spaces and correctness. wrote the logic on the github edit box directly.

[kitbdev]

queue_redraw just adds it to a queue to be redrawn, its fine to have in a loop. Regions can be nested or in other folded areas, so the unhighlighting does need to be a part of the loop.

All unfold_line() really does is find the folded area the line is in and unhides and unhighlights that area. With unfold all lines, we don't need to find each folded area since we know we want them all unfolded. So it just needs to make sure that all lines are unhidden and unhighlighted.

[tokengamedev]

Regions can be nested or in other folded areas, so the unhighlighting does need to be a part of the loop.

I get it, this is to handle the same bug when you unfold a particular instance. It is kind of bug from my perspective that when you expand one level, it expands all inside the level as level may not have been implemented, only relying on, line number to fold and unfold.

Unfold line - finds the folded area and unfolds it. unhighlighting is just an add on.

[tokengamedev]

As the issue is aware but I don't find this as a critical issue so closing it

[AThousandShips]

Keeping open until fixed

[GeminiSquishGames]

Is it maybe also related to the fact when using regions, if one is to use the Fold All command then unfold only the region, it unfolds all folded lines in the region? That's pretty annoying and prevents me from wanting to use regions as a code organization tool, but it's kind of necessary when scripts get large and have many sections of methods that don't need a whole new class definition. I don't want to make another issue if fixing this issue is part of the same bug. It might not be critical to engine usage but it is for easing the development flow in scripting. Otherwise, maybe just eliminate the script editor and replace it with a way to supply your own easily, if you also don't think it's important enough to fix at this stage. There really should be QoL fixes right next to critical, but it may be subjective and perhaps no one wants to work on it ATM.

(I've said fold so much in my head today, I'm reminded of the SquareOne TV: Dirk Niblick episode where Fold fluffs and Fluff folds—you probably had to be there)