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

AstroCalc: Add a filter by eclipse type #4054

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

10110111
Copy link
Contributor

@10110111 10110111 commented Jan 5, 2025

Description

This adds a way to select only particular types of eclipses to compute, e.g. only total, or only hybrid+annular. The UI might have been done better, but I'm not a UI designer :P

Screenshots

stellarium-011

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

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

Copy link

github-actions bot commented Jan 5, 2025

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.

@alex-w
Copy link
Member

alex-w commented Jan 5, 2025

Upper part of GUI in this tab is one for all subtabs and of course this filter is not applicable to lunar eclipses and to transits. Please move UI part of filter to bottom part of GUI.

@alex-w alex-w added the subsystem: astrocalc The issue is related to AstroCalc subsystem of planetarium... label Jan 5, 2025
@alex-w alex-w added this to the 25.1 milestone Jan 5, 2025
@10110111 10110111 force-pushed the eclipse-type-filter branch from b1d117f to 17d2280 Compare January 5, 2025 13:04
@10110111
Copy link
Contributor Author

10110111 commented Jan 5, 2025

I'm retaining them at the same place, but now this block is used both for general and local solar eclipses (hiding "hybrid" checkbox for locals), and hidden for other pages.

@10110111 10110111 force-pushed the eclipse-type-filter branch from 17d2280 to b51cbe8 Compare January 5, 2025 14:08
@alex-w
Copy link
Member

alex-w commented Jan 5, 2025

What about “jumping” of time range block when subtabs are changing?

@10110111
Copy link
Contributor Author

10110111 commented Jan 5, 2025

What about “jumping” of time range block when subtabs are changing?

Do you think it's critical? The tabs are mostly independent, aren't they?

@10110111 10110111 force-pushed the eclipse-type-filter branch from b51cbe8 to 7dbbc7b Compare January 6, 2025 06:11
@10110111
Copy link
Contributor Author

10110111 commented Jan 6, 2025

I've now extended it to lunar eclipses. So I think this placement in a common location is a good thing, and the jumpiness is not critical. Maybe just the aesthetics of the GUI are a bit suboptimal.

@10110111
Copy link
Contributor Author

10110111 commented Jan 6, 2025

Maybe this layout will be better:

stellarium-013

@alex-w
Copy link
Member

alex-w commented Jan 6, 2025

Maybe this layout will be better:

What about placement of new block between name of table and time range block?

@10110111
Copy link
Contributor Author

10110111 commented Jan 6, 2025

What about placement of new block between name of table and time range block?

IMO the filter by type is secondary to the time range, so the former shouldn't appear logically before the latter. Besides, I didn't like the two-checkbox-rows layout anyway, since it extends everything vertically in this HBox, leaving ugly space around. And putting the horizontal row in between will result in the name string being split into two lines (e.g. local eclipses title in Russian locale is quite long).
The new version seems coherent with both UI items being basically filters kept closely together.

Copy link
Member

@alex-w alex-w left a comment

Choose a reason for hiding this comment

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

OK

@gzotti
Copy link
Member

gzotti commented Jan 6, 2025

I agree the new boxes should not go between table title and date range. But what about after? Then the first line reads "Table of xxx eclipses from year YYYY to the next NN years" (could also go in one continuous label, just keep bold as now) with selector boxes behind (after a stretch, right adjusted). Saves one vertical line. Or is the Russian name of local eclipses really too long for that?

@alex-w
Copy link
Member

alex-w commented Jan 6, 2025

I agree the new boxes should not go between table title and date range. But what about after? Then the first line reads "Table of xxx eclipses from year YYYY to the next NN years" (could also go in one continuous label, just keep bold as now) with selector boxes behind (after a stretch, right adjusted). Saves one vertical line. Or is the Russian name of local eclipses really too long for that?

The current 2-line UI is good for translation to languages with "wide words"

@alex-w
Copy link
Member

alex-w commented Jan 6, 2025

Local eclipses:
stellarium-001

@10110111
Copy link
Contributor Author

10110111 commented Jan 6, 2025

Local eclipses:

And to add to this, note that "Annular" is "Кольцеобразное", one more long word.

@alex-w
Copy link
Member

alex-w commented Jan 6, 2025

@10110111 please add ability to save/restore filter options in/from config.ini file

@alex-w alex-w added the enhancement Improve existing functionality label Jan 6, 2025
Copy link

github-actions bot commented Jan 6, 2025

Hello @10110111!

Thank you for the suggested improvement.

@10110111
Copy link
Contributor Author

10110111 commented Jan 6, 2025

please add ability to save/restore filter options in/from config.ini file

If this is gonna be preserved, should it be per-page then? Or would a global state be sufficient?

@gzotti
Copy link
Member

gzotti commented Jan 6, 2025

Separate entries for partial/total lunar and solar please. And (as used elsewhere in AstroCalc) these settings should be stored immediately. On next call, the settings should appear as the last time user called the panel.

@alex-w
Copy link
Member

alex-w commented Jan 6, 2025

@10110111 did you have tested the saving/restoring feature? The config options look’s overcomplicated.

@10110111
Copy link
Contributor Author

10110111 commented Jan 7, 2025

did you have tested the saving/restoring feature? The config options look’s overcomplicated.

Sure I have tested it. What exactly looks overcomplicated to you? We have 3 tabs, with 4,3,3 checkboxes. All have been requested to be kept independent, thus we get 10 independent options in the config file:

[astrocalc]
eclipse_filter/global_solar/annular_enabled = true
eclipse_filter/global_solar/hybrid_enabled  = true
eclipse_filter/global_solar/partial_enabled = true
eclipse_filter/global_solar/total_enabled   = true
eclipse_filter/local_solar/annular_enabled  = true
eclipse_filter/local_solar/partial_enabled  = true
eclipse_filter/local_solar/total_enabled    = true
eclipse_filter/lunar/partial_enabled        = true
eclipse_filter/lunar/penumbral_enabled      = true
eclipse_filter/lunar/total_enabled          = true

@alex-w
Copy link
Member

alex-w commented Jan 7, 2025

OK, thanks!

@alex-w alex-w merged commit 4dabbbe into Stellarium:master Jan 7, 2025
15 checks passed
@10110111 10110111 deleted the eclipse-type-filter branch January 7, 2025 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality subsystem: astrocalc The issue is related to AstroCalc subsystem of planetarium...
Development

Successfully merging this pull request may close these issues.

3 participants