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.

Sign up for GitHub

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

Jump to bottom

Fix/input scale in UI #146

Merged
merged 16 commits into from
Sep 22, 2023
Merged

Fix/input scale in UI #146

merged 16 commits into from
Sep 22, 2023

Conversation

bordoray
Copy link
Collaborator

@bordoray bordoray commented Sep 1, 2023

Issue

close #144

テスト手順:Test

  • input scale on UI
  • test with web mercator and other crs

@bordoray bordoray requested a review from Kanahiro September 1, 2023 03:22
Kanahiro
Kanahiro requested changes Sep 4, 2023
View reviewed changes
Copy link
Member

@Kanahiro Kanahiro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

problem

mapcanvas is flickered like following movie:

2023-09-04.22.24.39.mov

codes

  • comments (this is not blocker to merge)

ui/main_dialog.py Outdated Show resolved Hide resolved
utils.py Outdated Show resolved Hide resolved
ui/main_dialog.py Outdated Show resolved Hide resolved
@bordoray
Copy link
Collaborator Author

bordoray commented Sep 11, 2023 •
edited
Loading

from this commitment, flickered issue looks not occurs

@bordoray
Copy link
Collaborator Author

@Kanahiro Okay I solved comments

@bordoray bordoray requested a review from Kanahiro September 11, 2023 03:52
Kanahiro
Kanahiro reviewed Sep 11, 2023
View reviewed changes
scale.py Outdated
return iface.mapCanvas().scale()


def set_map_extent_from_webmercator(scale: float):
Copy link
Member

Choose a reason for hiding this comment

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

This is not an abstraction.

#146 (comment)

I mean in the comment:

Suggested change
def set_map_extent_from_webmercator(scale: float):
def set_map_extent_from(scale: float, crs: QgsCoordinateReferenceSystem):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh right !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Kanahiro
Sorry I misunderstood your comment.
When other crs is defined, approach is different.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I'll push my codes.

@Kanahiro
Copy link
Member

Kanahiro commented Sep 11, 2023 •
edited
Loading

@bordoray
the flicker problem occurs even when newest codes.
the way to reproduce is same to following:
#146 (review)

  1. open plugin dialog
  2. set project-crs as EPSG:3857
  3. select zoom in plugin dialog
  4. drag mapcanvas (flicker!)

@bordoray
Copy link
Collaborator Author

Actually it flicker, only after set a scale on plugX UI

@bordoray bordoray requested a review from Kanahiro September 12, 2023 00:39
@bordoray
Copy link
Collaborator Author

@Kanahiro How it is?

@Kanahiro
Copy link
Member

@bordoray
the flicker looks remain still...
Did you reproduce that on your machine?

2023-09-19.9.26.03.mov

@bordoray
Copy link
Collaborator Author

@Kanahiro Sorry I misunderstood.
It still flickering, but I rearrange code to solve other comments.

comments (this is not blocker to merge)

I thought flickering is not blocker to merge and we will solve in separate issue, but I think understood now, that we have to solve flickering issue to merge, right?

@Kanahiro
Copy link
Member

the flicker is a blocker for me because:

  • a goal of this pull-request is to implement the feature to input scale in plugin UI.
  • but the feature has a bug
  • the code quality is also important but this is less important that above.

@Kanahiro
Copy link
Member

I have understood I mentioned "this is not blocker to merge" and you read this for the feature.
Sorry for occuring misunderstanding but I mean the codes (quality) is not blocker and the feature is blocker.

@bordoray
Copy link
Collaborator Author

Okay thanks for clarifying.
Sorry for misunderstood, I will debug now.

@bordoray
Copy link
Collaborator Author

bordoray commented Sep 19, 2023 •
edited
Loading

@Kanahiro Looks iface.mapCanvas().extentsChanged.disconnect() signal makes the map flickered.
https://github.com/MIERUNE/qgis-plugin-for-plugx/pull/146/files#diff-de91a3ce09f7dfdcba68f804df42972a7548fb2157542bd39dad4e6b18bf03dcR299
I am investigating for another approach, but any idea is welcomed ;)

EDIT : FYI this simple command launched in Python console made map flickered even without open plugin UI
image

@bordoray
Copy link
Collaborator Author

@Kanahiro Okay it should be fixed!

Kanahiro
Kanahiro approved these changes Sep 22, 2023
View reviewed changes
Copy link
Member

@Kanahiro Kanahiro left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks a lot.

@Kanahiro Kanahiro merged commit bf33b16 into main Sep 22, 2023
@Kanahiro Kanahiro deleted the fix/input-scale-in-ui branch September 22, 2023 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kanahiro Kanahiro Kanahiro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

set output scale in plugin dialog, and improve ui
2 participants
@bordoray @Kanahiro