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

Store opened search tabs between app restarts #22163

Merged
merged 1 commit into from
Jan 26, 2025
Merged

Store opened search tabs between app restarts #22163

merged 1 commit into from
Jan 26, 2025

Conversation

glassez
Copy link
Member

@glassez glassez commented Jan 14, 2025 •
edited
Loading

Closes #167.

001

@glassez glassez added Search engine Issues related to the search engine/search plugins functionality GUI GUI-related issues/changes labels Jan 14, 2025
@glassez glassez mentioned this pull request Jan 14, 2025
Closed
@glassez glassez force-pushed the search-session branch 2 times, most recently from c7b20a6 to 1595d69 Compare January 16, 2025 13:25
@glassez glassez force-pushed the search-session branch 2 times, most recently from 5017ae8 to 88c47d3 Compare January 22, 2025 06:59
@glassez glassez force-pushed the search-session branch 8 times, most recently from 4fae71f to 87b0c39 Compare January 24, 2025 13:15
@glassez glassez marked this pull request as ready for review January 24, 2025 13:16
@glassez glassez requested a review from a team January 24, 2025 13:16
@xavier2k6
Copy link
Member

Unrelated to this PR.......but how does a user know what the error is??

Screenshot 2025-01-24 175145

@thalieht
Copy link
Contributor

  1. I imagine it would be useful to be able to differentiate between empty tabs and non empty tabs when results are saved. Now they all have the checkmark icon.
  2. The Engine column (searchResult.engineName) is not restored after restart.
  3. Nitpick: In View menu and hotkeys (Alt+2, Alt+3), RSS is after Search but not in Options.
  4. I don't think it should say Search has finished in status bar for any restored tabs, with or without results.

@glassez
Copy link
Member Author

glassez commented Jan 25, 2025 •
edited
Loading

  1. I imagine it would be useful to be able to differentiate between empty tabs and non empty tabs when results are saved. Now they all have the checkmark icon.

I would not like to complicate this too much, at least in the initial implementation.

I don't think it should say Search has finished in status bar for any restored tabs, with or without results.

Showing just restored tabs as Finished looks strange to me too, so I changed them to not display any status (first two tabs):
20250125_105552

@glassez
Copy link
Member Author

glassez commented Jan 25, 2025

Nitpick: In View menu and hotkeys (Alt+2, Alt+3), RSS is after Search but not in Options.

Changed Options to follow View menu order.

@glassez

This comment was marked as resolved.

Sign in to view
@thalieht

This comment was marked as resolved.

Sign in to view
@glassez

This comment was marked as resolved.

Sign in to view
@thalieht
Copy link
Contributor

thalieht commented Jan 25, 2025 •
edited by glassez
Loading

Now Searching... is never shown :)
Also i can't tell if Search returned no results is broken in this PR (works in 5.0.3) or not because every search finishes with An error occurred during search..., same thing in master.

@xavier2k6
Copy link
Member

Now Searching... is never shown :) Also i can't tell if Search returned no results is broken in this PR (works in 5.0.3) or not because every search finishes with An error occurred during search..., same thing in master.

reference: #22163 (comment)

@thalieht
Copy link
Contributor

every search finishes with An error occurred during search..., same thing in master.

Scratch that. It's probably some problem with my self compiled builds. I went all the way back to the 5.0.0alpha1 commit and i can still reproduce it but i can't reproduce it in my distro's 5.0.3.

@glassez
Copy link
Member Author

glassez commented Jan 25, 2025

Unrelated to this PR.......but how does a user know what the error is??

I don't know. Search process just returns error code for some reason...

@glassez
Copy link
Member Author

glassez commented Jan 25, 2025

I went all the way back to the 5.0.0alpha1 commit and i can still reproduce it but i can't reproduce it in my distro's 5.0.3.

Perhaps the Search engine (python part) itself has some changes in v5.1 affecting this.

@glassez

This comment was marked as resolved.

Sign in to view
@glassez
Copy link
Member Author

glassez commented Jan 25, 2025

I went all the way back to the 5.0.0alpha1 commit and i can still reproduce it but i can't reproduce it in my distro's 5.0.3.

Perhaps the Search engine (python part) itself has some changes in v5.1 affecting this.

If so, it is most likely a regression (feature?) of #21098.
@Chocobo1, ping.

@thalieht
Copy link
Contributor

/Off topic

every search finishes with An error occurred during search...

It's probably some problem with my self compiled builds

I went all the way back to the 5.0.0alpha1 commit and i can still reproduce it but i can't reproduce it in my distro's 5.0.3.

I just remembered something that might be relevant: Unlike 5.0.3, i use my self compiled build in portable mode (profile folder).

@Chocobo1
Copy link
Member

Chocobo1 commented Jan 25, 2025 •
edited
Loading

If so, it is most likely a regression (feature?) of #21098.

It is a feature. Some of your plugins are definitely not working as expected. Although currently there are no debug messages in qbt to know about the details IIRC.

@xavier2k6
Copy link
Member

An error occurred during search...

official builds with only official plug-ins added/loaded & running in portable mode don't have this issue, only CI builds seem to display this problem, I went to a CI build which is still available from 3 months ago.

@xavier2k6
Copy link
Member

Testing plugins 1 by 1 & EZTV, TORLOCK & TORRENTS-CSV display the issue..... (plug-ins up-to-date)

Jackett Published On column is empty if I do a search, close/re-open qBittorrent & Published On column is populated.

Screenshot 2025-01-25 215305

Screenshot 2025-01-25 215328

@thalieht
Copy link
Contributor

Testing plugins 1 by 1 & EZTV, TORLOCK & TORRENTS-CSV display the issue..... (plug-ins up-to-date)

For me only torrents-csv has the problem.

@xavier2k6
Copy link
Member

Also, If I disable those 3x affected plugins & search with All plugins, it'll display the error - however if I choose Only enabled it'll give the check-mark.

@xavier2k6
Copy link
Member

xavier2k6 commented Jan 25, 2025 •
edited
Loading

Testing plugins 1 by 1 & EZTV, TORLOCK & TORRENTS-CSV display the issue..... (plug-ins up-to-date)

For me only torrents-csv has the problem.

@thalieht What version of python installed?? I'm 3.13.1/Windows 10

BTW....Search for example ubuntu on eztv, if you search something that is expected like S01 it'll give a check-mark.

@xavier2k6
Copy link
Member

Searching with S01 for each individual plugin:

Screenshot 2025-01-25 220646

@thalieht
Copy link
Contributor

@thalieht What version of python installed?? I'm 3.13.1/Windows 10

Same but linux.

BTW....Search for example ubuntu on eztv, if you search something that is expected like S01 it'll give a check-mark.

Search returned no results uses the same icon as error.

Maybe we should move this somewhere else... we're off topic.

@xavier2k6
Copy link
Member

For me only torrents-csv has the problem.

Can't seem to get any results at all with this plugin.

@xavier2k6

This comment was marked as off-topic.

Sign in to view
@glassez glassez merged commit 3978137 into qbittorrent:master Jan 26, 2025
14 checks passed
@glassez glassez deleted the search-session branch January 26, 2025 14:12
Chocobo1
Chocobo1 reviewed Jan 27, 2025
View reviewed changes
src/gui/search/searchwidget.cpp
SearchWidget::SearchWidget(IGUIApplication *app, QWidget *parent)
: GUIApplicationComponent(app, parent)
, m_ui {new Ui::SearchWidget()}
, m_ioThread {new QThread}
, m_dataStorage {new DataStorage(this)}
Copy link
Member

Choose a reason for hiding this comment

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

I'm getting this message in console: QObject::moveToThread: Cannot move objects with a parent

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm getting this message in console: QObject::moveToThread: Cannot move objects with a parent

Fixed by #22208.

@xavier2k6 xavier2k6 mentioned this pull request Jan 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Chocobo1 Chocobo1 Chocobo1 left review comments

@thalieht thalieht thalieht approved these changes

Assignees
No one assigned
Labels
GUI GUI-related issues/changes Search engine Issues related to the search engine/search plugins functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remember old searches (search history)
4 participants
@glassez @xavier2k6 @thalieht @Chocobo1