-
-
Notifications
You must be signed in to change notification settings - Fork 448
Support QTTabBar #2803
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
Support QTTabBar #2803
Conversation
WalkthroughWalkthroughThe recent updates to the Flow Launcher application improve its compatibility with the QTTabBar tool in Windows 10. Custom explorer configurations now support dynamic directory path replacements, allowing users to open directories in existing QTTabBar tabs. Additionally, UI elements have been visually refined, and language instructions for file manager configurations have been clarified. Changes
Sequence Diagram(s)Silent ignore. Assessment against linked issues
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to handle the scenario where the new OTTabBar entry should show up on existing user data, because the way the code loads the existing user settings it won't load the new entry in unless it's a fresh install.
I agree, but I don't know how to do it. I considered adding a separate setting for this item for existing user, but there was no proper location. (And since qtttabbar is not being maintained. so I didn't want to add a setting menu for this.) Let me ask chatgpt... |
Now, there are three ways to support this. I can work with 1 or 2.
3.If the user doesn't have qttabbar setting, it should be added automatically, and if it is already added, it should not be added again if the user subsequently deletes it. I don't think I can do this, and while it's probably possible, it results in an incredibly complex structure. Is there any other easy way to do this? This is entirely down to me structuring it incorrectly at the beginning, However, I don't know if it's worth adding such a structure for this utility. Given the importance of qttabbar support, I think my PR state is good enough. I can't do 3. After reviewing all 3, I still think what I proposed is the right way to go, but if you want to do 1 and 2 to add a qttabbar item to the list that almost all existing users will never use, that's fine. Just pick one and let me know. |
I'm closing that PR because I don't think my plan is clear and it's causing confusion. If I come up with a new idea, I'll write a new one, or others can make one. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
greater flexibility with custom explorer profile
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
Flow.Launcher/PublicAPIInstance.cs (1)
Line range hint
232-247
: Consider adding error handling for process start.Currently, there is no error handling if the process fails to start. Consider adding a try-catch block to handle potential exceptions.
try { using var process = new Process(); process.StartInfo = new ProcessStartInfo { FileName = explorerInfo.Path.Replace("%d", DirectoryPath), UseShellExecute = true, Arguments = FileNameOrFilePath is null ? explorerInfo.DirectoryArgument.Replace("%d", DirectoryPath) : explorerInfo.FileArgument .Replace("%d", DirectoryPath) .Replace("%f", Path.IsPathRooted(FileNameOrFilePath) ? FileNameOrFilePath : Path.Combine(DirectoryPath, FileNameOrFilePath) ) }; process.Start(); } catch (Exception ex) { LogException(nameof(PublicAPIInstance), "Failed to start process", ex); }
Thanks for helping to investigate how to get QTTabBar integrated. We just need to do a slight tweaking that will have less complexity, please see my commit. The commit will allow QTTabBar be added as a custom profile, which means there is no need to provide backwards support for a new default profile. We can add it like this: Tested:
|
@onesounds could you please help make the change to allow the 'Arg For Folder' and 'Arg For File' fields to be blank. |
@jjw24 Done. |
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
gitstream/estimated_time_to_review • add label gitstream/explain_code_experts • add explain code experts comment:
gitstream/percent_new_code • add comment gitstream/request_screenshot • add comment To activate these actions - merge this PR into the main branch Learn more on the gitStream Docs |
What's the PR
Detail
Test Cases