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

TUI fixes #3483

Merged
merged 9 commits into from Oct 30, 2023
Merged

TUI fixes #3483

merged 9 commits into from Oct 30, 2023

Conversation

gzotti
Copy link
Member

@gzotti gzotti commented Oct 29, 2023

Description

After my surprise about #3481 I decided to push this branch a few days earlier than anticipated.

This is not a complete fix/rethinking of the TUI plugin, just a few updates.
Keyboard action toggles for

  • show time
  • show object info

Still missing is any type of config GUI, at least with those two flag toggles.

Fixes # (issue)

Screenshots (if appropriate):

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • Housekeeping

How Has This Been Tested?

Test Configuration:

  • Operating system: Win11
  • Graphics Card: Geforce, but irrelevant

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (header file)
  • I have updated the respective chapter in the Stellarium User Guide
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@gzotti gzotti added enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash labels Oct 29, 2023
@gzotti gzotti added this to the 23.4 milestone Oct 29, 2023
@gzotti gzotti self-assigned this Oct 29, 2023
@gzotti gzotti added this to Needs triage in Didactic via automation Oct 29, 2023
@gzotti gzotti added this to Needs triage in Plugin via automation Oct 29, 2023
@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

This is an automatically generated QA checklist based on modified files.

@gzotti
Copy link
Member Author

gzotti commented Oct 29, 2023

Problem: I need a Windows install package with TUI tomorrow evening. What was the incantation?

@gzotti
Copy link
Member Author

gzotti commented Oct 29, 2023

Problem 2: The Regular expressions to remove the HTML decoration in StelObject::postProcessInfoString() is still not good, and it shows in the result. What to update, and where else does it appear (will something else break instead?)

@10110111
Copy link
Contributor

Problem: I need a Windows install package with TUI tomorrow evening. What was the incantation?

[publish] in commit message.

@10110111
Copy link
Contributor

The Regular expressions to remove the HTML decoration in StelObject::postProcessInfoString() is still not good, and it shows in the result.

What shows in the result? What is the input?

@gzotti
Copy link
Member Author

gzotti commented Oct 29, 2023

If you run this,
configure hotkey to toggle object info, then just select e.g. Jupiter and see the green insert.
Go into planetarium settings (F2/Tools) and activate gravity labels and disc viewport to see what I need to deliver.

A date should be visible to the visitors. That works. But the object info shows HTML extras. (<table style='...'>)

Ah -- you also need to activate F2/Extras/Use formatting.

@10110111
Copy link
Contributor

Well, the only tag I see slipping through is <table style='...'>, which isn't handled by any of the regexes. The one relevant to tables, tableRe2, doesn't match the style attribute.

@10110111
Copy link
Contributor

10110111 commented Oct 29, 2023

The table with a style can be removed with the following change (and I don't know what tableRe2 was supposed to stand for):

diff --git a/src/core/StelObject.cpp b/src/core/StelObject.cpp
index a1008d44e8..6fb81e9cc3 100644
--- a/src/core/StelObject.cpp
+++ b/src/core/StelObject.cpp
@@ -961,6 +961,8 @@ void StelObject::postProcessInfoString(QString& str, const InfoStringGroup& flag
     static const QRegularExpression brRe2("<br(\\s*/)?>\\s*");
     static const QRegularExpression tdRe("<td(\\w*)?>");
     static const QRegularExpression tableRe2("<table(\\w*)?>");
+    static const QRegularExpression tableRe3("<table style='[^']*'>");
+    static const QRegularExpression tableRe4("<table style=\"[^\"]*\">");
     str.replace(tableRe, "<table");
     // chomp trailing line breaks
     str.replace(brRe, "");
@@ -977,6 +979,8 @@ void StelObject::postProcessInfoString(QString& str, const InfoStringGroup& flag
         str.replace("<td>", "");
         str.replace("</tr>", "\n");
         str.replace(tableRe2, "");
+        str.replace(tableRe3, "");
+        str.replace(tableRe4, "");
         str.replace("</table>", "");
     }
     else if(!(flags&NoFont))

@gzotti
Copy link
Member Author

gzotti commented Oct 29, 2023

Well, the only tag I see slipping through is <table style='...'>, which isn't handled by any of the regexes. The one relevant to tables, tableRe2, doesn't match the style attribute.

Yes, same conclusion here. The "\w" is "word characters", right? Maybe spaces and quote signs should be added. I tried "<table(['\\\"\\w\\s]*)?>", that was not enough.

@10110111
Copy link
Contributor

10110111 commented Oct 29, 2023

I tried "<table(['\\\"\\w\\s]*)?>", that was not enough.

Right, because the style also contains hyphens, semicolons etc. It's just easier to handle it as "an opening quote, anything that's not a closing quote, a closing quote", for two variants of the quotes. You can put it into a single regex by using alternation.

@gzotti
Copy link
Member Author

gzotti commented Oct 29, 2023

Yes, that works great! Thanks a lot!

@gzotti
Copy link
Member Author

gzotti commented Oct 29, 2023

Another one:
str.replace("<td>", "");

Is this not covered by tdRe? And probably also here any style element like align should be removed.

@10110111
Copy link
Contributor

Is this not covered by tdRe?

Yes, it is. But again, what valid HTML tag does this regex represent aside from <td>?

@gzotti
Copy link
Member Author

gzotti commented Oct 29, 2023

<td> may also include style. See StelObject::getSolarLunarInfoString(). I will check other occurrences and deal with them the same way.

@gzotti
Copy link
Member Author

gzotti commented Oct 29, 2023

Ah yes, this could also match <tdfoo>. Not really good, but in effect it is only used in the HTML we are creating.

@10110111
Copy link
Contributor

10110111 commented Oct 29, 2023

Ah yes, this could also match <tdfoo>.

Well, this is the only kind of thing that this regex could match. There's a space missing between the words and the tag name. If you do use explicit style-matching regexes as you do in the new commit, this regex is just useless.

@gzotti
Copy link
Member Author

gzotti commented Oct 29, 2023

I replaced the indeed useless \\w now with \\s to catch simple cases of just added space. Currently it looks good.

@10110111
Copy link
Contributor

Now it makes sense. But the (\s*)? is redundant and is equivalent (unless you really want to capture) to \s*.

@gzotti
Copy link
Member Author

gzotti commented Oct 29, 2023

Right, it's only one element.

@gzotti gzotti marked this pull request as ready for review October 29, 2023 22:40
@gzotti gzotti merged commit 8e40930 into master Oct 30, 2023
16 of 17 checks passed
Didactic automation moved this from Needs triage to Done Oct 30, 2023
@gzotti gzotti deleted the tui-fixes branch October 30, 2023 11:53
Plugin automation moved this from Needs triage to Done Oct 30, 2023
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Nov 5, 2023
Copy link

github-actions bot commented Nov 5, 2023

Hello @gzotti!

Please check the fresh version (development snapshot) of Stellarium:
https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot

@alex-w alex-w removed the state: published The fix has been published for testing in weekly binary package label Dec 23, 2023
Copy link

Hello @gzotti!

Please check the latest stable version of Stellarium:
https://github.com/Stellarium/stellarium/releases/latest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash
Projects
Didactic
  
Done
Plugin
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants