Opened 13 years ago

Closed 12 years ago

#3061 closed patch (wontfix)

MSqlQuery.size() removal for libmythtv

Reported by: anonymous Owned by: Isaac Richards
Priority: minor Milestone: unknown
Component: mythtv Version: head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

this overrides #3060.

can someone close #3060?

Attachments (5)

libmythtv-size-removal.diff (45.4 KB) - added by anonymous 13 years ago.
patch.r12767.diff (45.2 KB) - added by luitjens 13 years ago.
updated patch to revision 12767
patch.r12767.2.diff (45.2 KB) - added by luitjens 13 years ago.
updated to r12767 and replaced a few .size() calls with .numRowsAffected(). usleep does this work for you?
msqlquerysizeremoval.diff.gz (17.6 KB) - added by usleepless@… 13 years ago.
msqlquerysizeremoval.diff.2.gz (16.5 KB) - added by anonymous 13 years ago.

Download all attachments as: .zip

Change History (20)

Changed 13 years ago by anonymous

Attachment: libmythtv-size-removal.diff added

comment:1 Changed 13 years ago by usleepless@…

i forgot to fill in "Reported by"

comment:2 Changed 13 years ago by david@…

The QT manual (3.3) says that query.exec() "Returns TRUE and sets the query state to active if the query was successful".

So couldn't things like "query.exec() && query.isActive() &&..." be simplified to "query.exec() && ..." while you're at it?

comment:3 Changed 13 years ago by usleepless@…

David,

thanks for thinking along. i already removed some of the isActive() calls, but i rather wait and see if this ticket is commited. i don't want to change too much on the first run.

if it commited, i will press on and submit ticket(s) for the rest of the code-base.

regards,

usleep

comment:4 Changed 13 years ago by luitjens

I have been using this patch for a day without problems. Also I looked over the patch and it appears to be correct. However, I think the patch should be updated to reflect David's comments before being committed as it is a simple change. Usleep could you update the patch to remove the unnecessary isActive() calls please?

Thanks, Justin

Changed 13 years ago by luitjens

Attachment: patch.r12767.diff added

updated patch to revision 12767

Changed 13 years ago by luitjens

Attachment: patch.r12767.2.diff added

updated to r12767 and replaced a few .size() calls with .numRowsAffected(). usleep does this work for you?

comment:5 Changed 13 years ago by luitjens

This patch has worked just fine. Tonight I recorded 9 programs in 2 hours while watching another recorded program with zero problems. I have also updated the patch to the latest head and removed a couple more .size() calls and replaced some of them with .numRowsAffected(). usleep is this function compatible for you?

The patch can be found here: http://svn.mythtv.org/trac/attachment/ticket/3061/patch.r12767.2.diff

comment:6 in reply to:  5 Changed 13 years ago by david@…

Replying to luitjens:

This patch has worked just fine. Tonight I recorded 9 programs in 2 hours while watching another recorded program with zero problems. I have also updated the patch to the latest head and removed a couple more .size() calls and replaced some of them with .numRowsAffected(). usleep is this function compatible for you?

Ok, I did a quick review of the patch:

  • I couldn't find any numRowsAffected calls in the patch.
  • There are still a lot of redundant isActive() calls left
  • The .size() removal has been taken too far. For instance, code like this is just plain ugly:
-        int qsize = query.size();
+        while(query.next())
+            qsize++

MySQL and PostgreSQL (and any other DB I've used with QT) support the .size() call, so I see no point in not using it where appropriate.

In order to not come across as a nag...I'll provide an updated patch later today or tomorrow.

comment:7 Changed 13 years ago by usleepless@…

David,

i will update it myself ( according to your remarks ).

how do i update the patch to revision revision 12767?

regards,

usleep

comment:8 in reply to:  7 Changed 13 years ago by david@…

Replying to usleepless@gmail.com:

how do i update the patch to revision revision 12767?

You should not have to do anything special, just make sure that you have an up-to-date SVN repo (use "svn update") before you create the patch.

comment:9 Changed 13 years ago by usleepless@…

i did "svn update", had one weird conflict.

i am unable to upload to trac ( rejecting spam: led )

how come?

regards,

usleep

Changed 13 years ago by usleepless@…

comment:10 Changed 13 years ago by luitjens

Sorry,

I must have attached the wrong patch. Also I agree that the qsize part is ugly. If numRowsAffected solves the size issue then we should use that.

comment:11 Changed 13 years ago by david@…

I've reviewed the latest patch, and first of all...beautiful. It removes around 700 unnecessary function calls. The diffstat says a lot about how much work it is:

 cardutil.cpp           |   37 ++++-----
 channel.cpp            |    2 
 channelbase.cpp        |   26 +++---
 channeleditor.cpp      |   17 +---
 channelsettings.cpp    |   16 ++-
 channelsettings.h      |   15 +--
 channelutil.cpp        |  100 +++++++++++++++---------
 customedit.cpp         |    8 -
 datadirect.cpp         |   15 +--
 dbcheck.cpp            |   14 +--
 dbox2channel.cpp       |    6 -
 dbox2epg.cpp           |    4 
 diseqc.cpp             |   20 ++--
 dtvchannel.cpp         |    6 -
 dtvmultiplex.cpp       |    2 
 dvbchannel.cpp         |    5 -
 eit.cpp                |    2 
 eitcache.cpp           |    4 
 eithelper.cpp          |    4 
 eitscanner.cpp         |    2 
 iptvchannel.cpp        |    2 
 jobqueue.cpp           |  104 ++++++++-----------------
 livetvchain.cpp        |    6 -
 mhi.cpp                |    4 
 mpegrecorder.cpp       |    2 
 profilegroup.cpp       |   16 +--
 progfind.cpp           |   24 +++--
 proglist.cpp           |   12 --
 programdata.cpp        |   34 +++-----
 programinfo.cpp        |  201 +++++++++++++++++++------------------------------
 recordingprofile.cpp   |   26 +++---
 remoteencoder.cpp      |    2 
 scanwizardhelpers.cpp  |    4 
 scheduledrecording.cpp |    9 --
 siscan.cpp             |   16 +--
 sourceutil.cpp         |   16 +--
 sr_items.cpp           |    2 
 sr_items.h             |    6 -
 storagegroup.cpp       |   12 +-
 transporteditor.cpp    |    8 -
 tv_play.cpp            |   14 +--
 tv_rec.cpp             |   42 ++++------
 videodev_myth.h        |   12 +-
 videoout_xv.cpp        |    3 
 videosource.cpp        |   31 +++----
 yuv2rgb.cpp            |   11 ++
 46 files changed, 436 insertions(+), 488 deletions(-)

It looks almost perfect, some things to fix though:

  • You introduce a spelling error in dbcheck.cpp (see the last line):
    -        if (!recordids.exec())
    -            return false;
     
    -        if (recordids.isActive() && recordids.size() > 0)
    +        if (recordids.ecec())
    
  • The changes to videodev_myth.h are completely unrelated to the SQL changes
  • Same thing goes for videoout_xv.cpp
  • Same thing goes for mpegrecorder.cpp
  • Same thing goes for yuv2rgb.cpp
  • You haven't resolved the conflict in channelutil.cpp (search that file for "<<<<" and you'll see it)

comment:12 Changed 13 years ago by Isaac Richards

Patches which obviously haven't even been compiled or tested by the submittor should NOT be getting posted to trac.

Changed 13 years ago by anonymous

comment:13 Changed 13 years ago by usleepless@…

fixed typo and actually compiled ( friday i was due to leave for the weekend, sorry ... ) removed the patches for videodev_myth.h, videoout_xv.cpp, mpegrecorder.cpp and yuv2rgb.cpp ( which were fbsd patches, sorry again )

comment:14 Changed 13 years ago by david@…

I've reviewed the new patch and it looks good, all the complaints I had have been resolved.

I hope it can get committed soon since a patch that touches this many files will bitrot quite quickly.

comment:15 Changed 12 years ago by greg

Resolution: wontfix
Status: newclosed

long dead patch which is now way way out of date. Closing

Note: See TracTickets for help on using tickets.