Skip to content

Take 2: Qt6 readiness: Use typed enums#4836

Closed
jonoomph wants to merge 1 commit intodevelopfrom
qt6-prep-enums
Closed

Take 2: Qt6 readiness: Use typed enums#4836
jonoomph wants to merge 1 commit intodevelopfrom
qt6-prep-enums

Conversation

@jonoomph
Copy link
Member

@jonoomph jonoomph commented Jun 23, 2022

Updating code base (especially Qt enums) to be compatible with Qt6. This is mostly work from @ferdnyc and PR #4833. I've made a few additional changes (formatting, bug fixes), but due to the older base of this PR (I think), I had trouble merging these changes back into the original PR.

Additional changes:

  • Removed some unused lines of code
  • Removed some unused imports
  • Reformatted a few small blocks of code
  • Refactored a few small blocks
  • Found some duplicate code and removed it

I also did a bunch of local testing with this branch, using Qt5, and did not see any breakages, errors, or stack traces.

…his is mostly work from ferdnyc and #4833. I've made a few additional changes (formatting, bug fixes)
@jonoomph
Copy link
Member Author

@ferdnyc
Copy link
Contributor

ferdnyc commented Jun 24, 2022

@jonoomph

Honestly? This doesn't sit right with me at all.

Never mind the pointlessness of leaving review comments on my other PR, then closing it before I could even address them. That's actually beside the point.

If you wanted to make changes to my commit, there are two different ways — no, wait, three different ways — you could do that without obliterating the attribution. (Proper git attribution, not a note in a commit message.)

  1. You could've created this branch, edited my PR to target it, and merged the PR to this branch
  2. You could've git cherry-picked the commit from the PR branch in my fork into this branch, after creating it.
  3. You could've used git format-patch -1 from a checkout of my PR branch, to export my commit to a patch file, then run git am 0001-Py-Qt6-readiness-Use-typed-enums.patch from this branch to re-import the commit.

Any of those would keep my ID attached to my changes.

Copy link
Collaborator

@JacksonRG JacksonRG left a comment

Choose a reason for hiding this comment

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

One line looks like it's missing an enum class. Other than that, looks good.

from PyQt5.QtGui import QPalette
p = QPalette()
p.setColor(QPalette.Highlight, Qt.green)
p.setColor(QPalette.Highlight, Qt.GlobalColor.green)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw other QPalette.Highlight replaced with QPalette.ColorRole.Highlight

@jonoomph
Copy link
Member Author

@jonoomph

Honestly? This doesn't sit right with me at all.

Never mind the pointlessness of leaving review comments on my other PR, then closing it before I could even address them. That's actually beside the point.

If you wanted to make changes to my commit, there are two different ways — no, wait, three different ways — you could do that without obliterating the attribution. (Proper git attribution, not a note in a commit message.)

  1. You could've created this branch, edited my PR to target it, and merged the PR to this branch
  2. You could've git cherry-picked the commit from the PR branch in my fork into this branch, after creating it.
  3. You could've used git format-patch -1 from a checkout of my PR branch, to export my commit to a patch file, then run git am 0001-Py-Qt6-readiness-Use-typed-enums.patch from this branch to re-import the commit.

Any of those would keep my ID attached to my changes.

@ferdnyc Please calm down, no need to get aggressive. Nobody is trying to steal your attribution. I am fine merging your previous PR, if you can pull my changes from this branch and fix all the issues with the original PR. I spent an hour trying to merge changes into your branch unsuccessfully (not to mention prioritized this work above my own), and thus, created this one as a last ditch effort to move this forward. So, you can criticize my lack of git skills, sure, but let's not be overly emotional and accusatory.

Closing this one for now (waiting on changes to the original)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants