Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#2277 closed Bug (fixed)

Run the Subtitle downloader in a separate thread

Reported by: xhmikosr Owned by: demi_alucard
Priority: normal Milestone: 1.6.2
Component: General Version: 1.6.1.4235
Severity: major Keywords:
Cc: Underground78, demi_alucard, XhmikosR Evaluation:

Description

Run the Subtitle downloader in a separate thread

Attachments (1)

patch.rar (46.4 KB) - added by demi_alucard 5 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by demi_alucard

  • Owner set to demi_alucard
  • Status changed from new to assigned

comment:2 Changed 5 years ago by demi_alucard

I already have something working. I'm just refactoring a little because the SubtitleDownloader code is a mess. I also added status messages telling the user if mpc-hc is downloading the list of subs, parsing the downloaded list, downloading the subtitles, warning if no subtitles were found and if the connection failed.

This is the first time I write something using threads, so I'll sure need some help with reviews.

comment:3 Changed 5 years ago by demi_alucard

Just a small preview for testing (patch+exe+pdb): http://www.mediafire.com/file/29hkp97y04t6fvv/worker_thread.rar

There is no support for translations and currently only the downloading of the list of available subtitles is run in a new thread. This means that opening the Subtitle Downloader does not make the player unresponsive anymore, but I still need to put the downloading of each selected subtitles in a worker thread.
Because of this, the Subtitle Downloader dialog still gets unresponsive when you click the download subs button just the same as before.

comment:4 Changed 5 years ago by underground78

The patch looks good to me.

I only have two minor remarks:

  • maybe instead of using UNREFERENCED_PARAMETER you can do something like that:
    LRESULT CSubtitleDlDlg::OnFailedConnection(WPARAM /*wParam*/, LPARAM /*lParam*/)
    
  • maybe you could rename OnHdnItemclickList1 to something a bit more explicit and replace &CSubtitleDlDlg::OnHdnItemclickList1 by just OnHdnItemclickList1.

comment:5 Changed 5 years ago by xhmikosr

I can only comment on the UNREFERENCED_PARAMETER macro which I prefer compared to commenting out the parameter.

I haven't tried the patch yet.

comment:6 Changed 5 years ago by demi_alucard

  • Cc XhmikosR added

Worker Thread Patch v2 (Base Revision 4713)

Changes:
Downloads lists of subtitles in a new thread.
Uses windows native interfaces available on NT5 or later to sort the columns. (does not depend on <algorithm> anymore.
Added a statusbar to the dialog.
Made the dialog width 100px smaller as per Xhmikosr request.
Changed the default column sizes to something more sensible. Now they will use the full dialog width.
Made the loading of column sizes from the settings a bit more robust, if the setting is corrupted, the columns will revert to the default sizes.
Removed compatibility flags for Windows 3.0 and older. (this is something that needs to be done to all other dialogs)
Reduced flicker.
Made the listview show tooltips for clipped text.

Please test and review the new english strings.

comment:7 follow-up: Changed 5 years ago by underground78

Have you included the changes to the RC file in the patch file? I can't open it on Linux.

Changed 5 years ago by demi_alucard

comment:8 in reply to: ↑ 7 Changed 5 years ago by demi_alucard

Replying to underground78:

Have you included the changes to the RC file in the patch file? I can't open it on Linux.

Oops, I forgot. Try this new archive.

comment:9 Changed 5 years ago by underground78

The patch seems fine but I can't really check in detail for now.

comment:10 Changed 5 years ago by demi_alucard

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed in r4734.

comment:11 Changed 5 years ago by xhmikosr

  • Component changed from New to Solved
  • Milestone changed from next release placeholder to next release
Note: See TracTickets for help on using tickets.