Am Mittwoch, den 10.03.2010, 01:29 +0100 schrieb manner.moe@gmx.de: > >Betreff : [linux-minidisc] autodetection of HiMD devices (Windows) > >Gesendet: 02.03.2010 19:06:45 > >An : "linux-minidisc@lists.fu-berlin.de" > >Von: "manner.moe@gmx.de" > > >----- Originale Nachricht ----- > >Hello, > > >here is my code for automatic detection of himd devices (windows > only, tested > Hi, > now I´ve also splitted the code into platform dependent and platform > independent parts. Thanks for your patch. I took some time for another review. Do you want me to change the stuff or do you want to implement the changes yourself? > diff --git a/qhimdtransfer/qhimddetection.cpp > b/qhimdtransfer/qhimddetection.cpp > new file mode 100644 > index 0000000..4d7b84c > --- /dev/null > +++ b/qhimdtransfer/qhimddetection.cpp > @@ -0,0 +1,31 @@ > +#include "qhimddetection.h" > +#include "ui_qhimddetection.h" I don't think we really need an .ui file for this dummy window. I would leave it out. > +QHiMDDetection::QHiMDDetection(QWidget *parent) : > + QDialog(parent), > + m_ui(new Ui::QHiMDDetection) > +{ > + m_ui->setupUi(this); > +} Which would kill the body of this function, and the upcall to the m_ui constructor. The upcall to the QDialog constructor would have to stay. > + > +QHiMDDetection::~QHiMDDetection() > +{ > + delete m_ui; > +} This function would go away. > +void QHiMDDetection::changeEvent(QEvent *e) > +{ > + QDialog::changeEvent(e); > + switch (e->type()) { > + case QEvent::LanguageChange: > + m_ui->retranslateUi(this); > + break; > + default: > + break; > + } > +} This function would also go away. > +void QHiMDDetection::closeEvent(QCloseEvent *event) > +{ > + disconnect(); > +} disconnect is QObject::disconnect( const char * signal = 0, const QObject * receiver = 0, const char * method = 0 ) Is calling this function really necessary? > diff --git a/qhimdtransfer/qhimddetection.h > b/qhimdtransfer/qhimddetection.h > new file mode 100644 > index 0000000..28f54a6 > --- /dev/null > +++ b/qhimdtransfer/qhimddetection.h > @@ -0,0 +1,48 @@ > +#ifndef QHIMDDETECTION_H > +#define QHIMDDETECTION_H > + > +#include <QtGui/QDialog> > + > +#define SONY 0x054c //known himd-mode product IDs > +#define MZ_NH1 0x017f > +#define MZ_NH3D 0x0181 > +#define MZ_NH900 0x0183 > +#define MZ_NH700 0x0185 > +#define MZ_NH600 0x0187 > +#define LAM_3 0x018a > +#define MZ_DH10P 0x01ea > +#define MZ_RH10 0x021a > +#define MZ_RH910 0x021c > +#define CMT_AH10 0x022d > +#define DS_HMD1 0x023d > +#define MZ_RH1 0x0287 > + > +namespace Ui { > + class QHiMDDetection; > +} Kill these three lines (and one of the empy lines) > + > +class QHiMDDetection : public QDialog { > + Q_OBJECT > + Q_DISABLE_COPY(QHiMDDetection) > +public: > + explicit QHiMDDetection(QWidget *parent = 0); > + virtual ~QHiMDDetection(); as indicated above, kill the destructor. > + virtual void scan_for_himd_devices(){}; > + > +protected: > + Ui::QHiMDDetection *m_ui; > + virtual void changeEvent(QEvent *e); Kill the last two lines to get rid of the UI stuff. > + virtual void closeEvent(QCloseEvent *event); > + > +protected slots: > + virtual void on_himd_busy(QString path){}; > + virtual void on_himd_idle(QString path){}; Please don't name slots on_... if they are not going to be magically connected to UI elements of that name. Just use "himd_busy"/"himd_idle". > + > +signals: > + void himd_found(QString path); > + void himd_removed(QString path); > +}; > + > +QHiMDDetection * createDetection(); > + > +#endif // QHIMDDETECTION_H I'm still unsure about this being the right model of OS-dependent/non-dependent split, but it seems to be a good start. It doesn't block your patch to go in. > diff --git a/qhimdtransfer/qhimddetection.ui > b/qhimdtransfer/qhimddetection.ui > new file mode 100644 > index 0000000..32e49b5 > --- /dev/null > +++ b/qhimdtransfer/qhimddetection.ui Kill the whole file! [file not cited] > diff --git a/qhimdtransfer/qhimdmainwindow.cpp b/qhimdtransfer/qhimdmainwindow.cpp > index 78a1786..e3d3e2a 100644 > --- a/qhimdtransfer/qhimdmainwindow.cpp > +++ b/qhimdtransfer/qhimdmainwindow.cpp > @@ -239,7 +239,9 @@ void QHiMDMainWindow::init_local_browser() > QStringList DownloadFileList; > localmodel.setFilter(QDir::AllDirs | QDir::Files | QDir::NoDotAndDotDot); > localmodel.setNameFilters(QStringList() << "*.mp3" << "*.wav" << "*.oma"); > - localmodel.setSorting(QDir::DirsFirst | QDir::Name); > + localmodel.setNameFilterDisables(false); > + localmodel.setReadOnly(false); > + localmodel.setRootPath("/home"); Are you *really* sure you need setRootPath()? This will break on Unix, and that's why we removed setRootPath from the old code. > ui->localScan->setModel(&localmodel); > QModelIndex curdir = localmodel.index(ui->updir->text()); > ui->localScan->expand(curdir); > @@ -250,6 +252,24 @@ void QHiMDMainWindow::init_local_browser() > ui->localScan->setColumnWidth(0, 350); > } > > +bool QHiMDMainWindow::autodetect_init() > +{ > + int k; > + > + k = QObject::connect(detect, SIGNAL(himd_found(QString)), this, SLOT(on_himd_found(QString))); > + k += QObject::connect(detect, SIGNAL(himd_removed(QString)), this, SLOT(on_himd_removed(QString))); > + > + if(!k) > + return false; > + > + QObject::connect(this, SIGNAL(himd_busy(QString)), detect, SLOT(on_himd_busy(QString))); > + QObject::connect(this, SIGNAL(himd_idle(QString)), detect, SLOT(on_himd_idle(QString))); > + > + detect->scan_for_himd_devices(); > + return true; > +} OK, fine. > + > + > void QHiMDMainWindow::open_himd_at(const QString & path) > { > QMessageBox himdStatus; > @@ -267,11 +287,18 @@ void QHiMDMainWindow::open_himd_at(const QString & path) > ui->himdpath->setText(path); > settings.setValue("lastHiMDDirectory", path); > > + if(settings.contains(path)) > + ui->statusBar->showMessage(settings.value(path, QString()).toString()); It seem you (ab)use the settings (which are meant to be persistent between QHiMDTransfer invocations) to store the recorder names. As I suggested below, the recorder names should be moved to the OS-independent detection code, so this gets more like: himd_device * dev = detection->find_by_path(path); if(dev) ui->statusBar->showMessage(dev->recorderName); > + else > + ui->statusBar->clearMessage(); > + > set_buttons_enable(1,0,0,1,1,1,1); > } > void QHiMDMainWindow::upload_to(const QString & UploadDirectory) > { > + emit himd_busy(ui->himdpath->text()); > + > QHiMDTrackList tracks = trackmodel.tracks(ui->TrackList->selectionModel()->selectedRows(0)); > > int allblocks = 0; > @@ -324,7 +351,8 @@ void QHiMDMainWindow::upload_to(const QString & UploadDirectory) > break; > } > uploadDialog->finished(); > - localmodel.refresh(); > + > + emit himd_idle(ui->himdpath->text()); > } Fine. > QHiMDMainWindow::QHiMDMainWindow(QWidget *parent) > @@ -333,12 +361,16 @@ QHiMDMainWindow::QHiMDMainWindow(QWidget *parent) > aboutDialog = new QHiMDAboutDialog; > formatDialog = new QHiMDFormatDialog; > uploadDialog = new QHiMDUploadDialog; > + detect = createDetection(); > ui->setupUi(this); > ui->updir->setText(settings.value("lastUploadDirectory", > QDir::homePath()).toString()); > set_buttons_enable(1,0,0,0,0,0,1); > init_himd_browser(); > init_local_browser(); > + ui->himd_devices->hide(); > + if(!autodetect_init()) > + ui->statusBar->showMessage(" autodetection disabled", 10000); > } > > QHiMDMainWindow::~QHiMDMainWindow() > @@ -393,6 +425,7 @@ void QHiMDMainWindow::on_action_Format_triggered() > > void QHiMDMainWindow::on_action_Connect_triggered() > { > + int index; > QString HiMDDirectory; > HiMDDirectory = settings.value("lastHiMDDirectory", QDir::rootPath()).toString(); > HiMDDirectory = QFileDialog::getExistingDirectory(this, > @@ -403,6 +436,20 @@ void QHiMDMainWindow::on_action_Connect_triggered() > if(HiMDDirectory.isEmpty()) > return; > > + index = ui->himd_devices->findText(HiMDDirectory); > + if(index == -1) > + { > + ui->himd_devices->addItem(HiMDDirectory); > + index = ui->himd_devices->findText(HiMDDirectory); > + } > + ui->himd_devices->setCurrentIndex(index); > + > + if(ui->himd_devices->isHidden()) > + { > + ui->himd_devices->show(); > + ui->himdpath->hide(); > + } > + > open_himd_at(HiMDDirectory); > } fine. > > @@ -426,3 +473,65 @@ void > QHiMDMainWindow::handle_selection_change(const QItemSelection&, const > QItem > ui->action_Upload->setEnabled(nonempty); > ui->upload_button->setEnabled(nonempty); > } > + > +void QHiMDMainWindow::on_himd_found(QString HiMDPath) > +{ > + int index; > + > + if(HiMDPath.isEmpty()) > + return; > + > + index = ui->himd_devices->findText(HiMDPath); > + if(index == -1) > + ui->himd_devices->addItem(HiMDPath); > + > + if(ui->himd_devices->isHidden()) > + { > + ui->himd_devices->show(); > + ui->himdpath->hide(); > + } > + > + if(ui->himdpath->text() == "(disconnected)") better to have a variable "connected" than to rely on texts, especially as they could be translated. Add a function is_open to QHiMDTracksModel that checks for himd being NULL, and use that one instead. > + { > + index = ui->himd_devices->findText(HiMDPath); > + ui->himd_devices->setCurrentIndex(index); > + open_himd_at(HiMDPath); > + } > + > +} > + > +void QHiMDMainWindow::on_himd_removed(QString HiMDPath) > +{ > + int index; > + > + if(HiMDPath.isEmpty()) > + return; > + if (ui->himdpath->text() == HiMDPath) > + { > + ui->himdpath->setText("(disconnected)"); use tr to make the text translatabnle. > + ui->statusBar->clearMessage(); > + trackmodel.close(); > + } > + > + index = ui->himd_devices->findText(HiMDPath); > + if(index != -1) > + { > + ui->himd_devices->removeItem(index); > + } Omit the braces. > + > + if(ui->himd_devices->count() == 0) > + { > + ui->himd_devices->hide(); > + ui->himdpath->show(); > + } > +} > + > +void QHiMDMainWindow::on_himd_devices_activated(QString device) > +{ > + open_himd_at(device); > +} > + > +void QHiMDMainWindow::closeEvent(QCloseEvent *event) > +{ > + detect->close(); > +} > diff --git a/qhimdtransfer/qhimdmainwindow.h b/qhimdtransfer/qhimdmainwindow.h > index 9bc9fc1..22b9772 100644 > --- a/qhimdtransfer/qhimdmainwindow.h > +++ b/qhimdtransfer/qhimdmainwindow.h > @@ -4,10 +4,11 @@ > #include <QtGui/QMainWindow> > #include <QtGui/QFileDialog> > #include <QtCore/QSettings> > -#include <QtGui/QDirModel> > +#include <QtGui/QFileSystemModel> > #include "qhimdaboutdialog.h" > #include "qhimdformatdialog.h" > #include "qhimduploaddialog.h" > +#include "qhimddetection.h" > #include "qhimdmodel.h" > #include "../libhimd/himd.h" > #include <tlist.h> > @@ -37,8 +38,9 @@ private: > QHiMDAboutDialog * aboutDialog; > QHiMDFormatDialog * formatDialog; > QHiMDUploadDialog * uploadDialog; > + QHiMDDetection * detect; > QHiMDTracksModel trackmodel; > - QDirModel localmodel; > + QFileSystemModel localmodel; > QSettings settings; > QString dumpmp3(const QHiMDTrack & trk, QString file); > QString dumpoma(const QHiMDTrack & trk, QString file); > @@ -47,9 +49,13 @@ private: > void set_buttons_enable(bool connect, bool download, bool upload, bool rename, bool del, bool format, bool quit); > void init_himd_browser(); > void init_local_browser(); > + bool autodetect_init(); > void open_himd_at(const QString & path); > void upload_to(const QString & path); > > +protected: > + void closeEvent(QCloseEvent *event); > + > private slots: > void on_action_Connect_triggered(); > void on_action_Format_triggered(); > @@ -60,6 +66,13 @@ private slots: > void on_localScan_clicked(QModelIndex index); > void on_upload_button_clicked(); > void handle_selection_change(const QItemSelection&, const QItemSelection&); > + void on_himd_found(QString path); > + void on_himd_removed(QString path); > + void on_himd_devices_activated(QString device); Same as for QHiMDDetection: don't use the magic "on_" prefix for something that is not bound to UI elements using the name. > + > +signals: > + void himd_busy(QString path); > + void himd_idle(QString path); > }; > > #endif // QHIMDMAINWINDOW_H > diff --git a/qhimdtransfer/qhimdmainwindow.ui > b/qhimdtransfer/qhimdmainwindow.ui > index 65f7483..149bd77 100644 > --- a/qhimdtransfer/qhimdmainwindow.ui > +++ b/qhimdtransfer/qhimdmainwindow.ui Changes to the ui file are OK. Snippled > diff --git a/qhimdtransfer/qhimdtransfer.pro > b/qhimdtransfer/qhimdtransfer.pro > index 1f0689d..1776b6b 100644 > --- a/qhimdtransfer/qhimdtransfer.pro > +++ b/qhimdtransfer/qhimdtransfer.pro Is reformatting this file needed? If yes, please make two seperate patches out of it by using git-commit after just reformatting. You can do that in a separate branch and rebase on it to get the other changes over, like this: # saves your current changes into a commit with the title "Win32 # autodetection git commit -m "Win32 autodetection" -a # generate a new branch "cleanup" based on the state before the last # commit, i.e. without your changes git branch cleanup HEAD^ # switch to the new branch git checkout cleanup <reformat qhimdtransfer.pro> # mark the changed qhimdtransfer.pro as ready-to-commit. The "-a" switch # to git commit used above marks all changed files as ready-to-commit, # so it was not needed to add these files by hand git add qhimdtransfer.pro # make a commit with a nice message git commit -m "Reformat qhimdtransfer.pro to match the formatting generated by (name of the tools you used)" # switch back to the master branch, where your Win32 autodetection patch # is committed. git checkout master # modify history in a way that your changes are based on the reformatting # you did in the cleanup branch. git rebase cleanup <you will get a conflict message in qhimdtransfer.pro . Edit this file, search for "<<<" and ">>>", fix it up. This means keeping the sections from the master branch and removing the sections from the cleanup branch> # mark the conflicts as resolved git add qhimdtransfer.pro # continue rebasing. git rebase --continue > +win32:SOURCES += qhimdwindetection.cpp Add a qhimddummydetection.cpp with a createDetection function that just returns 0 and write an else:SOURCES += qhimddummydetection.cpp line here. > diff --git a/qhimdtransfer/qhimdwindetection.cpp > b/qhimdtransfer/qhimdwindetection.cpp > new file mode 100644 > index 0000000..6c4a363 > --- /dev/null > +++ b/qhimdtransfer/qhimdwindetection.cpp > @@ -0,0 +1,517 @@ > +#include <QDebug> > +#include <QtCore/QSettings> > +#include <QList> > +#include <qhimddetection.h> > + > +#define WINVER 0x0500 > +#include <windows.h> > +#include <dbt.h> > +#include <setupapi.h> > +#include <initguid.h> // needed to handle GUIDs > +#include <ddk/ntddstor.h> // needed for handling storage devices > +#include <ddk/cfgmgr32.h> // needed for CM_Get_Child function > + > +struct win_himd_device { //information for > each found himd device > + bool is_busy; > + HANDLE devhandle; > + HDEVNOTIFY himdChange; > + QString path; > + bool md_inserted; > + QString recorder_name; > + }; Make a struct himd_device with the fields is_busy, path, md_inserted and recorder_name. Move that structure into qhimddetection.h. For C++ magic, please also add a virtual destructor ("virtual ~himd_device();") to the structure. Then use struct win_himd_device : himd_device { HANDLE devhandle; HDEVNOTIFY himdChange; } here. > + > +static const GUID GUID_IO_MEDIA_ARRIVAL = > + {0xd07433c0, 0xa98e, 0x11d2, {0x91, 0x7a, 0x00, 0xa0, 0xc9, 0x06, > 0x8f, 0xf3} }; > + > +static const GUID GUID_IO_MEDIA_REMOVAL = > + {0xd07433c1, 0xa98e, 0x11d2, {0x91, 0x7a, 0x00, 0xa0, 0xc9, 0x06, > 0x8f, 0xf3} }; > + > +static const GUID GUID_DEVINTERFACE_USB_DEVICE = > + {0xa5dcbf10, 0x6530, 0x11d2, {0x90, 0x1f, 0x00, 0xc0, 0x4f, 0xb9, > 0x51, 0xed} }; > + > + > +class QHiMDWinDetection : public QHiMDDetection { > + > +public: > + void scan_for_himd_devices(); > + > +protected: > + virtual void closeEvent(QCloseEvent *event); > + > +private: > + QSettings settings; > + HDEVNOTIFY hDevNotify; > + QList<win_himd_device *> device_list; This list should be changed to "QList<himd_device *> and moved to QHiMDDetection. Be sure to have it protected instead of private to give QHiMDWinDetection access to the device_list. > + void autodetect_close(); > + int find_device(HANDLE devhandle, QString path); This function is to be split into two functions, see below. > + QString get_deviceID_from_driveletter(char i); > + bool is_himddevice(QString devID, QString & name); > + bool identified(QString devpath, QString & name); Remove these three function from the object, make them free functions instead. They don't access anything inside QHiMDWinDetection, so they don't need to be members. > + void add_himddevice(QString path, QString name); > + void remove_himddevice(QString path); > + bool check_removal(HANDLE devhandle, QString & path); > + void add_himd(HANDLE devhandle); > + void remove_himd(HANDLE devhandle); > + HDEVNOTIFY register_mediaChange(HANDLE devhandle); > + void unregister_mediaChange(HDEVNOTIFY himd_change); > + bool winEvent(MSG * msg, long * result); > + QString FindPath(unsigned long unitmask); Make this a free function, too. As all the free functions are not used outside this file, make them static, like static QString FindPath(...) instead of QString QHiMDWinDetection::FindPath(...) > + > +protected slots: > + void on_himd_busy(QString path); > + void on_himd_idle(QString path); Move these functions/slots upwards to QHiMDDetection. It's not windows-specific what they do. And please remove the "on_". > + > +}; > + > + > +QHiMDDetection * createDetection() > +{ > + return new QHiMDWinDetection; > +} > + > +void QHiMDWinDetection::autodetect_close() > +{ > + while (!device_list.isEmpty()) > + remove_himddevice(device_list.at(0)->path); > + > + return; > +} > + > +void QHiMDWinDetection::scan_for_himd_devices() > +{ > + unsigned long drives = GetLogicalDrives(); > + char drive[] = "A:\\"; > + QString name, devID; > + > + for (; drive[0] <= 'Z'; ++drive[0]) > + { > + if (drives & 0x1) > + { > + if(GetDriveTypeA(drive) == DRIVE_REMOVABLE) > + { > + devID = get_deviceID_from_driveletter(drive[0]); > + if(!devID.isEmpty() && !devID.contains("Floppy", Qt::CaseInsensitive)) > + { > + if(is_himddevice(devID, name)) > + add_himddevice(QString(drive[0]) + ":/", name); > + } > + } > + } > + drives = drives >> 1; > + } > + return; > +} Looks fine. > +int QHiMDWinDetection::find_device(HANDLE devhandle, QString path) > +{ > + int i = 0; > + bool found = false; > + > + if (device_list.isEmpty()) > + return -1; > + if(!path.isEmpty()) > + { > + for (; i < device_list.size(); i++) > + { > + if(device_list.at(i)->path == path) > + { > + found = true; > + break; > + } > + } > + } This function is essentially two functions. One is find_by_path, the other one is find_by_handle. The find_by_path-Code is independant of Windows stuff, and thus should be moved to QHiMDDetection instead of laying around here. Furthermore, I would not return the index, but the object pointer, and you can just "return ..." instead of setting found and breaking. You might want to overload the find_by_path code from QHiMDDetection in this class by a function that return win_himd_device instead of himd_device like this: win_himd_device* QHiMDWinDetection::find_by_path(QString path) { return static_cast<win_himd_device*>(QHiMDDetection::find_by_path(path); } > + else if(devhandle != NULL) > + { > + for (; i < device_list.size(); i++) > + { > + if(device_list.at(i)->devhandle == devhandle) > + { > + found = true; > + break; > + } > + } > + } This part gets a bit more complicated, as the generic device list is of type QList<himd_device>, not QList<win_himd_device>, so make a helper function (part of QHiMDWinDetection) like this: win_himd_device* QHiMDWinDetection::win_device_by_idx(int i) { return static_cast<win_himd_device *>(device_list.at(i)); } Then you use win_device_by_idx(i) instead of device_list.at(i). > + > + if (found) > + return i; > + > + return -1; > +} > + > +QString QHiMDWinDetection::get_deviceID_from_driveletter(char i) > +{ > + char subkey[] = "\\DosDevices\\X:"; > + DWORD valuesize; > + HKEY key; > + int res; > + QString devname; > + > + subkey[12] = i; > + res = RegOpenKeyExA( HKEY_LOCAL_MACHINE, "SYSTEM\\MountedDevices", NULL, KEY_QUERY_VALUE, &key); > + if(res != ERROR_SUCCESS) > + return QString(); > + > + if(RegQueryValueExA(key, subkey, NULL, NULL, NULL, &valuesize) == ERROR_SUCCESS) > + { > + char *value = new char[valuesize]; > + res = RegQueryValueExA(key, subkey, NULL, NULL,(LPBYTE) value, &valuesize); > + if(res == ERROR_SUCCESS) > + { > + devname = QString::fromUtf16((ushort*)value); > + devname.remove(0,4); // modify devname to make a valid device ID > + devname.truncate(devname.indexOf("{") -1); > + devname = devname.toUpper(); > + devname.replace("#", "\\"); > + } > + delete[] value; > + } > + RegCloseKey(key); > + return devname; > +} Looks fine, but make it a static free function, as explained above. > +bool QHiMDWinDetection::is_himddevice(QString devID, QString & name) > +{ > + DEVINST devinst; > + DEVINST devinstparent; > + unsigned long buflen; > + QString recname, devicepath; > + > + CM_Locate_DevNodeA(&devinst, devID.toAscii().data(), NULL); > + CM_Get_Parent(&devinstparent, devinst, NULL); > + > + if(devID.contains("RemovableMedia", Qt::CaseInsensitive)) // on Windows XP: get next parent device instance > + CM_Get_Parent(&devinstparent, devinstparent, NULL); > + > + CM_Get_Device_ID_Size(&buflen, devinstparent, 0); > + wchar_t *buffer = new wchar_t[buflen]; > + CM_Get_Device_ID(devinstparent, buffer, buflen, 0); > + devicepath = QString::fromWCharArray(buffer); > + delete[] buffer; > + > + if(identified( devicepath, recname)) > + { > + name = recname; > + return true; > + } > + return false; > +} Same comment. > +bool QHiMDWinDetection::identified(QString devpath, QString & name) > +{ > + int vid = devpath.mid(devpath.indexOf("VID") + 4, 4).toInt(NULL,16); > + int pid = devpath.mid(devpath.indexOf("PID") + 4, 4).toInt(NULL,16); > + > + if (vid != SONY) > + return false; > + > + switch (pid) > + { > + case MZ_NH1: > + name = "SONY MZ-NH1"; > + break; > + case MZ_NH3D: > + name = "SONY MZ-NH3D"; > + break; > + case MZ_NH900: > + name = "SONY MZ-NH900"; > + break; > + case MZ_NH700: > + name = "SONY MZ-NH700 / MZ-NH800"; > + break; > + case MZ_NH600: > + name = "SONY MZ-NH600(D)"; > + break; > + case LAM_3: > + name = "SONY LAM-3"; > + break; > + case MZ_DH10P: > + name = "SONY MZ-DH10P"; > + break; > + case MZ_RH10: > + name = "SONY MZ-RH10"; > + break; > + case MZ_RH910: > + name = "SONY MZ-RH910"; > + break; > + case CMT_AH10: > + name = "SONY CMT-AH10"; > + break; > + case DS_HMD1: > + name = "SONY DS-HMD1"; > + break; > + case MZ_RH1: > + name = "SONY MZ-RH1"; > + break; > + default: return false; > + } > + return true; > +} And again the same comment. > +void QHiMDWinDetection::add_himddevice(QString path, QString name) > +{ > + if (find_device(NULL, path) >= 0) > + return; > + > + win_himd_device * new_device = new win_himd_device; > + int k; > + char drv[] = "\\\\.\\X:"; > + QByteArray device = "\\\\.\\PHYSICALDRIVE"; > + char file[] = "X:\\HI-MD.IND"; > + DWORD retbytes; > + HANDLE hdev; > + STORAGE_DEVICE_NUMBER sdn; > + OFSTRUCT OFfile; > + > + drv[4] = path.at(0).toAscii(); > + > + hdev = CreateFileA(drv, NULL , FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, > + OPEN_EXISTING, 0, NULL); Do you really need FILE_SHARE_WRITE here? Also one might check (but we I am going to commit as it is now) whether it is possible to obtain the device number without opening the physical device, which is probably only possible as Administrator. Maybe it works without Administrator access, too, for removable devices. In that case, the code is fine as it is now. > + if(hdev == INVALID_HANDLE_VALUE) > + return; > + > + k = DeviceIoControl(hdev, IOCTL_STORAGE_GET_DEVICE_NUMBER, NULL, 0, &sdn, sizeof(sdn), &retbytes, NULL); > + CloseHandle(hdev); > + if(k != 0) > + device.append(QString::number(sdn.DeviceNumber)); > + > + new_device->devhandle = CreateFileA(device.data(), NULL , FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, > + OPEN_EXISTING, 0, NULL); Again: Do you really need FILE_SHARE_WRITE here? > + if(new_device->devhandle == INVALID_HANDLE_VALUE) > + return; > + > + new_device->himdChange = register_mediaChange(new_device->devhandle); > + new_device->is_busy = false; > + new_device->path = path; > + new_device->recorder_name = name; > + > + settings.setValue(new_device->path, new_device->recorder_name); Please don't abuse settings for stuff that's not intended to be permanent. I pointed out a different solution above. While you clear the settings on exit, they remain if the program crashes. > + > + file[0] = path.at(0).toAscii(); > + if(OpenFile(file, &OFfile, OF_EXIST) != HFILE_ERROR) > + { > + new_device->md_inserted = true; > + emit himd_found(new_device->path); > + qDebug() << "himd device at " + new_device->path + " added (" + new_device->recorder_name + ")"; > + } > + else > + { > + qDebug() << "himd device at " + new_device->path + " added (" + new_device->recorder_name + ")" + " ; without MD"; > + new_device->md_inserted = false; > + } > + > + device_list.append(new_device); > + > + return; > + > +} Looks fine otherwise. > + > +void QHiMDWinDetection::remove_himddevice(QString path) > +{ > + int i = find_device(NULL, path); As find_by_path now returns a pointer (if you follow my suggestion), this becomes: win_himd_device * dev = find_by_path(path); Use "dev->" instead of "device_list.at(i)." > + if (i < 0) > + return; if(!dev) return; > + > + unregister_mediaChange(device_list.at(i)->himdChange); > + > + if (device_list.at(i)->devhandle != NULL) > + CloseHandle(device_list.at(i)->devhandle); > + > + if(settings.contains(device_list.at(i)->path)) > + settings.remove(device_list.at(i)->path); > + emit himd_removed(device_list.at(i)->path); > + > + qDebug() << "himd device at " + device_list.at(i)->path + " removed (" + device_list.at(i)->recorder_name + ")"; > + > + delete device_list.at(i); > + device_list.removeAt(i); > +} This last call will become device_list.removeAll(dev); > +bool QHiMDWinDetection::check_removal(HANDLE devhandle, QString & path) > +{ > + int i = find_device(devhandle, NULL); > + if (i < 0) > + return false; > + > + path = device_list.at(i)->path; > + return device_list.at(i)->is_busy; > +} This does two things at once - it looks up the path and returns the busy state. I would kill this function completely and to the lookup in the calling function. You will then see a bug: If the devhandle is not found, path is not initialized, so remove_himddevice("") is called. It does not really harm, but it makes no sense. > +void QHiMDWinDetection::add_himd(HANDLE devhandle) > +{ > + int i = find_device(devhandle, NULL); > + if (i < 0) > + return; > + > + if(device_list.at(i)->md_inserted != true) > + { > + device_list.at(i)->md_inserted = true; > + emit himd_found(device_list.at(i)->path); > + qDebug() << "himd device at " + device_list.at(i)->path + " : md inserted"; > + } > + return; > +} If you follow the suggested interface change, also here you have to change from index-based access to pointer-basd access. > +void QHiMDWinDetection::remove_himd(HANDLE devhandle) > +{ > + int i = find_device(devhandle, NULL); > + if (i < 0) > + return; > + > + if(device_list.at(i)->md_inserted != false) > + { > + device_list.at(i)->md_inserted = false; > + emit himd_removed(device_list.at(i)->path); > + qDebug() << "himd device at " + device_list.at(i)->path + " : md removed"; > + } > + return; > +} Same comment... > +HDEVNOTIFY QHiMDWinDetection::register_mediaChange(HANDLE devhandle) > +{ > + DEV_BROADCAST_HANDLE filter; > + > + ZeroMemory( &filter, sizeof(filter) ); > + filter.dbch_size = sizeof(DEV_BROADCAST_HANDLE); > + filter.dbch_devicetype = DBT_DEVTYP_HANDLE; > + filter.dbch_handle = devhandle; > + filter.dbch_eventguid = GUID_IO_MEDIA_ARRIVAL; // includes GUID_IO_MEDIA_REMOVAL notification > + > + return RegisterDeviceNotification( this->winId(), &filter, DEVICE_NOTIFY_WINDOW_HANDLE); > + > +} Fine. > +void QHiMDWinDetection::unregister_mediaChange(HDEVNOTIFY himd_change) > +{ > + if(himd_change != NULL) > + UnregisterDeviceNotification(himd_change); > +} Fine, too. > +bool QHiMDWinDetection::winEvent(MSG * msg, long * result) > + { > + QString name, devID, path ; > + const int DBT_CUSTOMEVENT = 0x8006; Make this a global constant and declare it near the GUIDs. > + > + if(msg->message == WM_DEVICECHANGE) > + { > + PDEV_BROADCAST_HDR pHdr = (PDEV_BROADCAST_HDR )msg->lParam; > + switch(msg->wParam) > + { > + case DBT_DEVICEARRIVAL : > + { > + if(pHdr->dbch_devicetype == DBT_DEVTYP_VOLUME) > + { > + PDEV_BROADCAST_VOLUME pHdrv = (PDEV_BROADCAST_VOLUME)pHdr; > + path = FindPath(pHdrv->dbcv_unitmask); > + devID = get_deviceID_from_driveletter(path.at(0).toAscii()); > + if(!devID.isEmpty()) > + { > + if(is_himddevice(devID, name)) > + { > + qDebug() << "Message:DBT_DEVICEARRIVAL for drive " + path; > + add_himddevice(path, name); > + } > + } > + } > + break; > + } > + case DBT_DEVICEREMOVECOMPLETE : > + { > + if(pHdr->dbch_devicetype == DBT_DEVTYP_VOLUME) > + { > + PDEV_BROADCAST_VOLUME pHdrv = (PDEV_BROADCAST_VOLUME)pHdr; > + path = FindPath(pHdrv->dbcv_unitmask); > + qDebug() << "Message:DBT_DEVICEREMOVECOMPLETE for drive " + path; > + remove_himddevice(path); > + } > + break; > + } Do we really need to handle DEVICEREMOVECOMPLETE? You already called remove_himddevice on DEVICEQUERYREMOVE if you consented. > + case DBT_DEVICEQUERYREMOVE : > + { > + if(pHdr->dbch_devicetype & DBT_DEVTYP_HANDLE) > + { > + PDEV_BROADCAST_HANDLE pHdrh = (PDEV_BROADCAST_HANDLE)pHdr; > + if(check_removal(pHdrh->dbch_handle, path)) > + { > + *result = BROADCAST_QUERY_DENY; > + qDebug() << "Message:DBT_DEVICEQUERYREMOVE for drive " + path + " denied: transfer in progress"; > + return true; > + } > + else > + { > + qDebug() << "Message:DBT_DEVICEQUERYREMOVE requested"; > + remove_himddevice(path); > + } > + } > + break; > + } > + case DBT_CUSTOMEVENT : > + { > + if(pHdr->dbch_devicetype & DBT_DEVTYP_HANDLE) > + { > + PDEV_BROADCAST_HANDLE pHdrh = (PDEV_BROADCAST_HANDLE)pHdr; > + if (pHdrh->dbch_eventguid == GUID_IO_MEDIA_ARRIVAL) > + { > + qDebug() << "Message:DBT_CUSTOMEVENT - GUID_IO_MEDIA_ARRIVAL"; > + add_himd(pHdrh->dbch_handle); > + break; > + } > + if (pHdrh->dbch_eventguid == GUID_IO_MEDIA_REMOVAL) > + { > + qDebug() << "Message:DBT_CUSTOMEVENT - GUID_IO_MEDIA_REMOVAL"; > + remove_himd(pHdrh->dbch_handle); > + break; > + } > + } > + break; > + } > + default: return false; // skip unknown/unused messages > + } > + *result = TRUE; > + return true; > + } > + return false; > + } > +QString QHiMDWinDetection::FindPath (unsigned long unitmask) > +{ > + char i; > + > + for (i = 0; i < 26; ++i) > + { > + if (unitmask & 0x1) > + break; > + unitmask = unitmask >> 1; > + } > + return QString(i + 'A') + ":/"; > +} Make that a static free function. > +void QHiMDWinDetection::closeEvent(QCloseEvent *event) > +{ > + disconnect(); Call "QHiMDDetection::closeEvent()" instead of copying what's in there. Then you don't have to change here if the OS-independent part needs some changes in closeEvent(). > + autodetect_close(); > +} > + > +// slots > + > +void QHiMDWinDetection::on_himd_busy(QString path) > +{ > + int i = find_device(NULL, path); > + if (i < 0) > + return; > + > + device_list.at(i)->is_busy = true; > + qDebug() << "himd device at " + device_list.at(i)->path + " : device busy, starting transfer"; > +} > + > +void QHiMDWinDetection::on_himd_idle(QString path) > +{ > + int i = find_device(NULL, path); > + if (i < 0) > + return; > + > + device_list.at(i)->is_busy = false; > + qDebug() << "himd device at " + device_list.at(i)->path + " : device idle, transfer complete"; > +} These two function can be moved upwards to QHiMDDetection, as they don't do anything specific to Windows. Regards, Michael Karcher
Attachment:
signature.asc
Description: Dies ist ein digital signierter Nachrichtenteil