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

Added GUI to manually entering FOV #3093

Merged
merged 15 commits into from
Mar 15, 2023
Merged

Added GUI to manually entering FOV #3093

merged 15 commits into from
Mar 15, 2023

Conversation

alex-w
Copy link
Member

@alex-w alex-w commented Mar 10, 2023

Description

In this pull request I've proposes the optional GUI (available through hotkey F9) to manually entering the field of view (simlar to Date/Time window). Of course this tool changes FOV only - no rotation!

Parially fix #3013 (issue)

Screenshot

stellarium-003

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

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

@alex-w alex-w added this to the 23.1 milestone Mar 10, 2023
@github-actions github-actions bot requested review from 10110111 and gzotti March 10, 2023 09:14
@alex-w alex-w added the feature Entirely new feature label Mar 10, 2023
@github-actions
Copy link

github-actions bot commented Mar 10, 2023

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.

@10110111
Copy link
Contributor

"Use decimal degrees" option should be honored.

@10110111
Copy link
Contributor

10110111 commented Mar 10, 2023

Also, I think the UX would be best if the FOV field in the bottom panel became a button that would open this dialog (and have a tooltip to hint at the F9 shortcut).

The auto-sliding of the panel can be a bit of a nuisance though when you target this button and it jumps away from the cursor.

@alex-w
Copy link
Member Author

alex-w commented Mar 10, 2023

"Use decimal degrees" option should be honored.

OK, this is good point.

@alex-w
Copy link
Member Author

alex-w commented Mar 10, 2023

Also, I think the UX would be best if the FOV field in the bottom panel became a button that would open this dialog (and have a tooltip to hint at the F9 shortcut).

The auto-sliding of the panel can be a bit of a nuisance though when you target this button and it jumps away from the cursor.

Sorry, I disagree.

@alex-w
Copy link
Member Author

alex-w commented Mar 11, 2023

ok, it’s done.

@10110111 @gzotti please review the feature.

@axd1967 please check it too

@10110111
Copy link
Contributor

Why show both D°M'S" and D.D° when it's not done this way in any other angle entry field (e.g. in Location dialog)?

@alex-w
Copy link
Member Author

alex-w commented Mar 11, 2023

Why show both D°M'S" and D.D° when it's not done this way in any other angle entry field (e.g. in Location dialog)?

Why not? :)

Of course I can add code to change the UI to follow "Use decimal degrees” option on-the-fly, but… 1) the FOV value on the bottom bar are configurable and I don’t want to add the all possible combinations; 2) Date/Time window has in addition to the date and time JD/MJD - here is similar feature; 3) someone can use this tool as convertor between DMS and DD values.

Or you expect to see one field of input for FOV value (like as in Location dialog)?

@10110111
Copy link
Contributor

Or you expect to see one field of input for FOV value (like as in Location dialog)?

Yes, this is definitely what I expected. Otherwise why do we even have this checkbox in the Tools dialog?

I don't quite like the current two-formats form, but OK, let's see what others think.

@gzotti
Copy link
Member

gzotti commented Mar 11, 2023

Do we really need a full new dialog class, css addition and precious hotkey for such rarely used fine-tweak functionality? A simple AngleSpinBox in ViewSettings/Sky/Projection should be enough IMHO. Please avoid code bloat! Most users (all-1?) will continue to use regular zoom and maybe the ten configurable FOV shortcuts.

If you think this additional dialog is necessary, I have a few remarks:

  • It seems too wide and could be made more compact.
  • It should then have fixed (minimal) size.
  • when I spin the arcminutes with mouse wheel, the zero seems to block advancing if rotated slowly, but I can "crash through the wall" if I rotate the mouse wheel fast enough. The arrow buttons seem to never go through the zero.

@Atque
Copy link
Contributor

Atque commented Mar 11, 2023

  1. someone can use this tool as convertor between DMS and DD values.

I think this is a useful feature, so I vote for showing both in the same window.

@10110111
Copy link
Contributor

I think this is a useful feature, so I vote for showing both in the same window.

Purporting a completely unrelated dialog for such a converter is really bad design. Not only because the purposes are mixed so that using the converter has a side effect of changing the scene, but also:

  • a subsequent redesign can break down the secondary purpose;
  • discoverability of such a tool is poor.

Besides, when done in a single-format way, if you enter an angle in the current format and then toggle the setting in Tools dialog, the conversion would happen automatically, just as it is now in Location dialog.

Do we really need a full new dialog class, css addition and precious hotkey for such rarely used fine-tweak functionality?

+1 about the hotkey, and then my suggestion to use a panel button for this would help avoid spending a key.

As for the separate class, I think it's completely OK to use a new class since it improves modularity while letting one connect spinboxes with each other, leaving this invisible to other code.

But the CSS additions, albeit hacky (as most CSS-based styles in Qt), is basically the same design as in DateTimeDialog, as you might notice by its look (and by the same problem of horizontal bloating).

Most users (all-1?) will continue to use regular zoom and maybe the ten configurable FOV shortcuts

All users will continue using the regular zoom, but some of them will also be happy to be able to set the exact desired zoom level on the fly.

@alex-w alex-w marked this pull request as draft March 11, 2023 12:27
@alex-w alex-w modified the milestones: 23.1, 23.2 Mar 11, 2023
@gzotti
Copy link
Member

gzotti commented Mar 11, 2023

All users will continue using the regular zoom, but some of them will also be happy to be able to set the exact desired zoom level on the fly.

Yes, but a simple AngleSpinBox could do the same with far less added code.

@axd1967
Copy link
Contributor

axd1967 commented Mar 12, 2023

looking good.

  1. the UI should be opened by right-clicking on the status bar FOV field, similar to the behaviour of right-clicking in the bottom toolbar. this is to stay in line with the philosophy of opening related dialogs via the toolbar. both UI elements are intimately related. see also right click in toolbar to open plugin config dialog #1638 as well as Added GUI to manually entering FOV #3093 (comment). this might require extra coding/separate issue, as this logic is not yet present for the status bar
  2. the mouse wheel action is a bit confusing. an improvement is to ensure that the behaviour is the same whether inside or outside the F9 dialog. an improvement is a checkbox in the F2 (of F9) dialog to inverse the zoom behaviour of the main window so that zooming requires the same mouse wheel actions.
  3. I'm not sure F9 is a good way to spend a precious shortcut on this less used feature; I would use a more distant key sequence, such as e.g. Alt-F9
  4. could not replicate auto-sliding window behaviour (I use a laptop)

@10110111
Copy link
Contributor

the UI should be opened by right-clicking on the status bar FOV field, similar to the behaviour of right-clicking in the bottom toolbar

Right-clicking in the bottom toolbar triggers a side function ("configure"), while left-clicking toggles the plugin. Doing it the same way with FOV doesn't make sense, since the primary purpose of this button would be to configure FOV (we can't "toggle the field of view"). And if the button highlights as a button and left-clicking doesn't do anything, it's poor UX, and it won't make the user expect that right-clicking might work.

could not replicate auto-sliding window behaviour

It's the panel that slides, not a window. If you have the bottom panel always expanded (this is toggled by a button in the corner), then you'll not have this problem.

@gzotti
Copy link
Member

gzotti commented Mar 12, 2023

It would be an added primary dialog of the same rank as the time dialog, so a high-valued F9 keystroke (so far unused) and even button in the left bar would be called for. However, I really see no need for the whole dialog with all required plumbing and boilerplating when the same rarely used action can be added easily in an obvious location of an existing dialog. I would rather allow plugins with large GUIs take left-bar buttons (E.g. Scenery3D, Oculars), but this demand has been remedied by plugin display buttons with right-click config action in the bottom bar. (We should probably add right-click actions to the leftmost bottom bar buttons to show the respective tab in the view dialog!)
Adding buttons to the purely text-driven output ("status line") in the sliding bottom bar requires thorough redesign of the menu system. Currently it is possible to configure which fields are shown in the text output.

@10110111
Copy link
Contributor

10110111 commented Mar 12, 2023

Well, one option is to just add a spinbox near the "Custom FoV limit" in View dialog, naming it something like "Current FoV". It just must not interfere with possible very small values of navigation/min_fov, i.e. not force its digit limits on normal wheel-driven control. Maybe even set the number of fractional digits in the spinbox to fit this minimum.

@alex-w
Copy link
Member Author

alex-w commented Mar 12, 2023

Seems to work, but has one annoyance: pressing Enter after typing the value opens Skylight Details dialog (I have Skylight/enable_gui set to true).

Hmm… I don’t understand why it happens

@gzotti
Copy link
Member

gzotti commented Mar 12, 2023

Seems to work, but has one annoyance: pressing Enter after typing the value opens Skylight Details dialog (I have Skylight/enable_gui set to true).

Hmm… I don’t understand why it happens

This may be the age-old issue with UI elements having focus for the Enter key. (See also #1971.) I almost always only operate the GUI with the mouse, so I almost never press enter or tab...

@alex-w
Copy link
Member Author

alex-w commented Mar 13, 2023

This may be the age-old issue with UI elements having focus for the Enter key. (See also #1971.) I almost always only operate the GUI with the mouse, so I almost never press enter or tab...

No, this is unrelated issue.

P.S. I found and fixed the issue.

@10110111
Copy link
Contributor

Your fix will break the intention of #1971. This is more a workaround for some other problem than a fix.
I had a similar problem when I was adding ShowMySky support into the Atmosphere dialog, and I didn't have to make anything unfocusable to fix it. Need to recall what I did instead...

@alex-w
Copy link
Member Author

alex-w commented Mar 13, 2023

Your fix will break the intention of #1971. This is more a workaround for some other problem than a fix. I had a similar problem when I was adding ShowMySky support into the Atmosphere dialog, and I didn't have to make anything unfocusable to fix it. Need to recall what I did instead...

focusPolicy::NoFocus for pushButtonAtmosphereDetails

@10110111
Copy link
Contributor

Not really, this part came in long before me, in 916d26b.

@alex-w
Copy link
Member Author

alex-w commented Mar 13, 2023

Not really, this part came in long before me, in 916d26b.

This is true black magic 🤬

@10110111
Copy link
Contributor

OK, recalled: this was my solution.

@alex-w
Copy link
Member Author

alex-w commented Mar 13, 2023

OK, recalled: this was my solution.

Well, this solution in the code already, but when I reset focus settings for atmosphere dialog button, then I see “standard” behavior - opening atmosphere dialog when some spinbox is filled by ‘Enter’

@alex-w
Copy link
Member Author

alex-w commented Mar 13, 2023

Stop! You are proposed add eventFilter to ViewDialog?

@10110111
Copy link
Contributor

You are proposed add eventFilter to ViewDialog?

Yes, I proposed to add a filter to ViewDialog and reject Enter and Return keys. They are never used for accepting this dialog anyway (unlike Esc, which rejects it).

@alex-w
Copy link
Member Author

alex-w commented Mar 14, 2023

Yes, I proposed to add a filter to ViewDialog and reject Enter and Return keys. They are never used for accepting this dialog anyway (unlike Esc, which rejects it).

No success. Probably I doing something wrong. Could you check it on your side?

@10110111
Copy link
Contributor

Probably I doing something wrong

You might have forgotten to install the event filter: simply overriding the method is not enough.

Anyway, I've pushed the version that works for me.

@alex-w
Copy link
Member Author

alex-w commented Mar 15, 2023

You might have forgotten to install the event filter: simply overriding the method is not enough.

Yes, this was correct point :-/

Anyway, I've pushed the version that works for me.

You have forgotten remove the focus property from "atmosphere" button.

@gzotti @10110111 This feature may be added into 23.1 release if it OK for you.

Copy link
Member

@gzotti gzotti left a comment

Choose a reason for hiding this comment

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

Works for me, thanks!

@alex-w alex-w modified the milestones: 23.2, 23.1 Mar 15, 2023
@alex-w alex-w merged commit ef13ee4 into master Mar 15, 2023
@alex-w alex-w deleted the ui/fov branch March 15, 2023 09:46
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Mar 19, 2023
@github-actions
Copy link

Hello @alex-w!

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 Mar 27, 2023
@github-actions
Copy link

Hello @alex-w!

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
feature Entirely new feature
Development

Successfully merging this pull request may close these issues.

finer grained zooming - manual plate solving
5 participants