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

Prepare icon and UI imports for qt6 QGis builds #250

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

merydian
Copy link
Collaborator

No description provided.

@koebi koebi added the easy fix Issues that are rather quick to be resolved label Aug 16, 2024
Copy link
Collaborator

@koebi koebi left a comment

Choose a reason for hiding this comment

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

Is there some documentation available on how qt6 handles things?

I am mostly confused by the removal of the resources*-files and the *UI.py-files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this whole file change? I assume b/c of line endings?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the license of this compatible with ours?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are importing a rather large chunk of code here, of which only 3 small functions are used.
I suggest to reduce the import to the functions in question.

Comment on lines +194 to +197
if not os.path.exists(path):
return path

return path
Copy link
Collaborator

Choose a reason for hiding this comment

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

This branching is essentially useless?

Comment on lines 189 to 193
path = os.path.join(
os.path.dirname(__file__),
'..',
'gui',
file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is highly dependent on the code layout of the plugin - in our case, it works, but it feels way more generic than it is?

return QIcon(path)

@staticmethod
def get_icon_svg(icon: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this reads like it should do something with svg, the code does nothing - and rather has the functionality I'd expect in get_icon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like more context on the deletion of this file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like more context on the deletion of this file.

@koebi koebi mentioned this pull request Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy fix Issues that are rather quick to be resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants