Fixed dialog windows tiling like main windows on tiling window managers. #464
Conversation
CyanVoxel
added
Type: Bug
|
|
|
I am genuinely out of words about that, the program worked just fine on my end with Qt.Dialog and it didn't seem to print any warnings or anything different from otherwise. Whatever though, inheriting QDialog instead of QWidget seems to work fine as well, at least I hope it does. |
|
@ChloeZamorano can you please fix the failing pipeline? |
CyanVoxel
added
Status: Changes Requested
|
@ChloeZamorano Any updates on this? |
|
@CyanVoxel Hi, sorry, I forgot about this and hadn't used Github in a while (Been using Codeberg for a while now). No updates whatsoever, but let me elaborate a little. So the stuff in the issue itself is all correct as far as I can tell, and both things I tried doing to fix it actually worked fine on my machine. I just don't know what this "failing pipeline" means on a functional level, I don't really ever work with interpreted languages, and I don't remember exactly what the errors in the "checks" were (Which are not showing up anymore for some reason?) but I think it said something about the class being missing? In any case, I'm pretty sure the changes I made should work fine according to the documentation I found online, and it did when I tested it on a real case scenario. So unless it's some dependency versioning shenanigans or I just messed up massively and used the wrong API? Because with how Python works, I think two different APIs for the same thing could potentially have interchangeable components? I just don't really know what actually happened here and my memory's bad, but I'd be glad to help out in some way if it's still an issue in the main repo. |
|
@ChloeZamorano Since this PR is quite a bit behind Also make sure that you're formatting and checking your code with Ruff and MyPy, as talked about in the CONTRIBUTING.md. The "failing pipeline" is in reference to our automated CI checks for these on the GitHub side, nothing to do with interpreted languages. |
This commit was made in order to account for users who may already have
a Python installation that isn't 3.12.
A good real world scenario where this may be a problem is users running
on rolling release Linux distributions, such as Arch Linux which
provides bleeding edge updates in a weekly basis.
Continuing with Arch Linux as an example, it provides Python 3.13, which
the current version of TagStudio doesn't seem to work with, an issue I
encountered myself this morning.
In some systems like Windows, this may not matter, but it is an issue
nonetheless, and users using a package manager centric system can't be
expected to downgrade/upgrade their Python version for TagStudio, as
this may cause issues in their system.
The easiest way to solve this is with pyenv, which enables users to
install other Python versions independent from each other, aside from
the system install, and allows specifying a version to use for a
directory (And its subdirectories), the current terminal session, or the
entire system if desired.
Here is a short, technical and objective summary of the changes:
- Updated `.gitignore` to to ignore `.python-version` which pyenv uses.
- Updated CONTRIBUTING.md to refer to pyenv for versioning issues.
- pyenv is listed as a prerequisite, specifying in parenthesis that it's only for when your Python install isn't 3.12.
- The "Creating a Python Virtual Environment" section now has an additional step at the beginning, to check that the current Python installation is 3.12.
- As part of this added step, there are steps to use pyenv to install Python 3.12 separately from the existing Python installation, and activate it for the TagStudio root folder, in case it's not the appropriate version.
- The numbering of the other steps was offset accordingly.
- The "Manually Launching (Outside of an IDE)" section now refers to the above first step, should the user encounter some kind of error.
Some users may not be using the default shell for their system, and Linux users are the most likely to do this, though this can be seen in macOS as well. People will often use shells like fish which is not POSIX compliant but works very well and has great auto-completion, it's very user friendly. The reason why I care about this is that the instructions specify to use .venv/bin/activate which is a bash script and will not run on fish or csh, and if you take a look at the script, indeed it says it will only run with bash. Python will provide alternate scripts for other shells, and at least on my system (CachyOS), those are scripts for Powershell, bash, fish and csh. People using these alternate shells, who don't know this may be confused when running the script referenced in CONTRIBUTING.md and seeing it fail completely. The only real change in this commit was adding under the "Linux/macOS" command in step 3 (step 2 prior to my last commit) of the "Creating a Python Virtual Environment" section, a short instruction to switch to the default shell, or use the appropriate script for the user's preferred shell if any (Since, at least on my system, there aren't scripts for every shell, for example there is no activate.zsh file).
|
After snooping around the internet, and messing with Ruff and MyPy, I think it's just never gonna pass a test without patching PySide6 itself?? I looked around, and triple check that I'm following the right documentation for the right version of the right API, and it seems fine, and still the program will run fine with no warnings or anything and do what I expect it to do with these windows, but still it won't pass MyPy. I looked around my
I also looked around for similar issues and came across these examples: To me, it looks like it's a case of PySide6 missing type hints for apparently several things that do function fine; using I guess using Also, running PyTest gave me some errors that are unrelated, even with no changes whatsoever from upstream, and I don't know what that's actually about, if not some mismatch of implementation or something, just thought I'd bring that up because I did rebase to the latest upstream commit. So here's an screenshot of an small bit of that, and I guess just ask if that's supposed to happen? I also committed some minor changes to the documentation, just regarding using different Python versions with pyenv and venv activation scripts for shells that aren't bash. I'll reopen the pull request when we decide on what to do about the MyPy error with the window flags, if anything, but for now I think I'll just leave this comment here. |
|
Could you commit your changes so I could see what's going on with the PR? It's currently empty for me so I can't test or investigate. You can also open the PR as a draft instead of having it closed. This may be an issue where we can just use As for the pytests, they shouldn't be giving those errors if you haven't purposely modified them. There's a |
Set the Qt.Dialog flag for modal windows, such as "Add Field" so that they will be seen as dialogs in the backend. This change is unlikely to be at all noticeable in systems like Windows, but for users running tiling window managers like bspwm or Sway, this will prevent these modal windows from being tiled alongside the main window, instead they will be floating on top, which is the expected behaviour seen on floating window managers, like the ones used by Windows and macOS. Added self.setWindowFlag(Qt.Dialog, on=True) # type: ignore to the following files: - tagstudio/src/qt/modals/add_field.py - tagstudio/src/qt/modals/delete_unlinked.py - tagstudio/src/qt/modals/drop_import.py - tagstudio/src/qt/modals/file_extension.py - tagstudio/src/qt/modals/fix_dupes.py - tagstudio/src/qt/modals/fix_unlinked.py - tagstudio/src/qt/modals/folders_to_tags.py - tagstudio/src/qt/modals/mirror_entities.py - tagstudio/src/qt/widgets/paged_panel/paged_panel.py - tagstudio/src/qt/widgets/panel.py - tagstudio/src/qt/widgets/progress.py Note that without adding # type: ignore, MyPy *will* give out an error, presumably because PySide6 is missing type hints for several things, and this may *or* may not be a justifiable use of # type: ignore, which throws the MyPy type checking for that line out the window. Ref: TagStudioDev#392, TagStudioDev#464
|
I made the corresponding changes and committed them to my fork, now I'll make dinner though, so I probably won't reply until tomorrow. I also checked the PyTest thing and yeah, sure enough that does work perfectly fine with |
|
@CyanVoxel any updates on the mypy ignore thing? |
|
Hello! Sorry for the delay, I've been busy getting things shaped up for the current pre-releases. The good news is that yes, it looks like that mypy is incorrect here and Since |
|
Excellent, if I did miss any modals that should be dialogues, it's because the way I looked for them was by searching for Also, if you got an spare pendrive, you can just boot your main machine off of that in some distro with a graphical installer, like Mint or some of the Arch forks. It's not gonna be recorded into the pendrive, but I know at least some of those let you install packages in the live image. Otherwise, a virtual machine may suffice, even on an slow machine, a default Arch install shouldn't be hard to virtualize. I can also go fetch my friends to see if they wanna test the pull request, which I probably will later today. I'm also hoping for input on the small additions I made to the documentation, though maybe that should be a separate pull request. |
There's some good additions in there, but yes that would be better served in a new PR if possible. I could give some input here and now or I could wait for if you'd like to split that off into a new PR |
|
I'd like to have input on it and then split it to a dedicated PR, so that PR can be a single commit in contrast to the mess this one ended up being. |
…791) * docs: refer to pyenv in CONTRIBUTING.md This commit was made in order to account for users who may already have a Python installation that isn't 3.12. A good real world scenario where this may be a problem is users running on rolling release Linux distributions, such as Arch Linux which provides bleeding edge updates in a weekly basis. Continuing with Arch Linux as an example, it provides Python 3.13, which the current version of TagStudio doesn't seem to work with, an issue I encountered myself this morning. In some systems like Windows, this may not matter, but it is an issue nonetheless, and users using a package manager centric system can't be expected to downgrade/upgrade their Python version for TagStudio, as this may cause issues in their system. The easiest way to solve this is with pyenv, which enables users to install other Python versions independent from each other, aside from the system install, and allows specifying a version to use for a directory (And its subdirectories), the current terminal session, or the entire system if desired. Here is a short, technical and objective summary of the changes: - Updated `.gitignore` to to ignore `.python-version` which pyenv uses. - Updated CONTRIBUTING.md to refer to pyenv for versioning issues. - pyenv is listed as a prerequisite, specifying in parenthesis that it's only for when your Python install isn't 3.12. - The "Creating a Python Virtual Environment" section now has an additional step at the beginning, to check that the current Python installation is 3.12. - As part of this added step, there are steps to use pyenv to install Python 3.12 separately from the existing Python installation, and activate it for the TagStudio root folder, in case it's not the appropriate version. - The numbering of the other steps was offset accordingly. - The "Manually Launching (Outside of an IDE)" section now refers to the above first step, should the user encounter some kind of error. * docs: refer to alternate shells in CONTRIBUTING.md Some users may not be using the default shell for their system, and Linux users are the most likely to do this, though this can be seen in macOS as well. People will often use shells like fish which is not POSIX compliant but works very well and has great auto-completion, it's very user friendly. The reason why I care about this is that the instructions specify to use .venv/bin/activate which is a bash script and will not run on fish or csh, and if you take a look at the script, indeed it says it will only run with bash. Python will provide alternate scripts for other shells, and at least on my system (CachyOS), those are scripts for Powershell, bash, fish and csh. People using these alternate shells, who don't know this may be confused when running the script referenced in CONTRIBUTING.md and seeing it fail completely. The only real change in this commit was adding under the "Linux/macOS" command in step 3 (step 2 prior to my last commit) of the "Creating a Python Virtual Environment" section, a short instruction to switch to the default shell, or use the appropriate script for the user's preferred shell if any (Since, at least on my system, there aren't scripts for every shell, for example there is no activate.zsh file). * fix: modal windows now have the Qt.Dialog flag Set the Qt.Dialog flag for modal windows, such as "Add Field" so that they will be seen as dialogs in the backend. This change is unlikely to be at all noticeable in systems like Windows, but for users running tiling window managers like bspwm or Sway, this will prevent these modal windows from being tiled alongside the main window, instead they will be floating on top, which is the expected behaviour seen on floating window managers, like the ones used by Windows and macOS. Added self.setWindowFlag(Qt.Dialog, on=True) # type: ignore to the following files: - tagstudio/src/qt/modals/add_field.py - tagstudio/src/qt/modals/delete_unlinked.py - tagstudio/src/qt/modals/drop_import.py - tagstudio/src/qt/modals/file_extension.py - tagstudio/src/qt/modals/fix_dupes.py - tagstudio/src/qt/modals/fix_unlinked.py - tagstudio/src/qt/modals/folders_to_tags.py - tagstudio/src/qt/modals/mirror_entities.py - tagstudio/src/qt/widgets/paged_panel/paged_panel.py - tagstudio/src/qt/widgets/panel.py - tagstudio/src/qt/widgets/progress.py Note that without adding # type: ignore, MyPy *will* give out an error, presumably because PySide6 is missing type hints for several things, and this may *or* may not be a justifiable use of # type: ignore, which throws the MyPy type checking for that line out the window. Ref: #392, #464 * fix: I forgot to run ruff format * revert: changes to docs and gitignore This revert is for moving the referenced changes to their own pull request. Refs: 15f4f38, a25ef8c * docs: other shells and pyenv in CONTRIBUTING.md This commit changes CONTRIBUTING.md to refer to pyenv in the case of issues with Python versions, as well as downloading the correct version off of the Python website alternatively. It also now elaborates on the process of running the Python virtual environment on Linux and macOS, by referencing activation scripts for alternative shells such as fish and CSH. The table added to CONTRIBUTING.md is taken from the official Python documentation: https://docs.python.org/3.12/library/venv.html * revert: accidental inclution of #464 Refs: a51550f, 5854ccc
|
Any updates on this? Really, I would just like it to be merged so I can have the main branch always updated in my bin directory with this fix. |
Sorry, I was under the impression that you were going to rebase/update this PR since it's quite far behind main now. There's also been new modal windows added that aren't currently covered by this PR. In the meantime, someone over on the Discord pulled this to test it out and noted that it wasn't making a different on their machine after applying the changes to Could also be user error, but I'm just passing the information along. They mentioned they're on niri which is a wayland tiling compositor I'm not familiar with, so the issue could also just be isolated to that compositor if there is indeed an issue. |
|
Right, sorry for being kind of reactionary about this, if that wording makes sense. I just synced my fork to look into the new modals, and I'm gonna be trying this and other applications in that compositor to see what's going on, and hopefully figure out a solution. It says in the README that they have floating windows as of 25.01 so if this person hasn't updated in some time, then that would be why, but I doubt it. |
I could not find in the documentation a list of what files are for dialog windows or anything along those lines, so I searched for instances of
self.setWindowModalityand as far as I'm seeing, it looks like that's all of them? In any case, all I did was addself.setWindowFlags(Qt.Dialog)to the init functions of every modal I found that calls setWindowModality, the files affected are:Do let me know if I missed any or if some of these shouldn't be changed, as I'm not familiar with this code base or this style of making GUIs.
This fix has not been tested on Wayland yet, but based on the assumption that Qt behaves the same on both display servers, it should work the same, as this is a very basic feature that both display servers should have.
This fix is for issue #392
For any discussions or comments, use my Discord (chloevz) if a quick reply is desired, as I don't tend to check Github daily.