4.7.2 has been released

[Dirk Hohndel]

I'm expecting 4.7.3 in a few weeks, so please keep the fixes coming.
4.7.2 fixed too many REAL problems to wait until things seemed all settled.

75 commits in 9 days is a rather respectable pace!

Sources are uploaded, tags are pushed, Windows, Mac and various Linux binaries 
are building.
AppImage is still missing.

I'll write an announcement for our website and Facebook / G+ in a moment.

Thanks to everyone who helped squash the bugs and make Subsurface better!

/D
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Import data from Scubapro Aladin Sport Matrix

[Berthold Stoeger]

On Dienstag, 31. Oktober 2017 11:29:00 CET Jef Driesen wrote:

> I believe you have the model number wrong. I think it should be 0x17,
> and your test seems to confirm that:

Well, I put a printf directly after "rc = scubapro_g2_transfer (device, 
cmd_model, sizeof (cmd_model), model, sizeof (model));" and guess what?
I got both, 0x17 *and* 0xa5 (the latter much more commonly). There seems to be 
some race. Will investigate further - tomorrow.

But when manually sending 0x10 manually with my test program, I consistently 
get a 0x17, which therefore seems to be the correct model. Sigh.

Berthold
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Thread related crash on failed dive computer download

[Berthold Stoeger]

On Dienstag, 31. Oktober 2017 21:00:37 CET Berthold Stoeger wrote:
> So in the attachment is a different approach: don't output errors if not in
> the main thread. Someone who generates a thread is responsible of flushing
> the errors later. Sounds much less brittle to me.

It works in my limited test-case and I generated a pull request:
https://github.com/Subsurface-divelog/subsurface/pull/753

Fixing the race condition should not be too hard.

Berthold

___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Thread related crash on failed dive computer download

[Berthold Stoeger]

Hi Dirk,

On Dienstag, 31. Oktober 2017 20:41:32 CET Dirk Hohndel wrote:
> On Tue, Oct 31, 2017 at 08:10:32PM +0100, Berthold Stoeger wrote:
> > To be honest, apart from the errors mentioned above, I'm not even sure
> > that
> > the idea behind the code is sound. If you want, I can send an
> > updated/cleaned up patch, but I have a bad feeling with this one. There
> > probably should at least be a non-varargs version.
> 
> The idea that the thread collects its messages and that the caller of the
> thread then initiates their display when it is done seems sound to me.
> 
> I'd really like a version that's good enough to use in 4.7.2 - and I'm
> open to further refinement once that's released.

I still think the approach is fundamentally broken/brittle. You have to 
identify which calls are made from the GUI thread and which are not. Who knows 
if I got the right report_error()s? What about errors that happen only in few 
cases?

So in the attachment is a different approach: don't output errors if not in 
the main thread. Someone who generates a thread is responsible of flushing the 
errors later. Sounds much less brittle to me.

This means that the membuffer in errorhelper.c has to be protected by a mutex 
or something (not an expert in threads). But that race was there before, 
wasn't it?

What do you think?

Bertholddiff --git a/desktop-widgets/downloadfromdivecomputer.cpp b/desktop-widgets/downloadfromdivecomputer.cpp
index 637f6034..b875b4a7 100644
--- a/desktop-widgets/downloadfromdivecomputer.cpp
+++ b/desktop-widgets/downloadfromdivecomputer.cpp
@@ -220,6 +220,10 @@ void DownloadFromDCWidget::updateState(states state)
 	// got an error
 	else if (state == ERROR) {
 		timer->stop();
+
+		// Show messages that worker thread produced.
+		MainWindow::instance()->showErrors();
+
 		QMessageBox::critical(this, TITLE_OR_TEXT(tr("Error"), thread.error), QMessageBox::Ok);
 		markChildrenAsEnabled();
 		progress_bar_text = "";
diff --git a/desktop-widgets/mainwindow.cpp b/desktop-widgets/mainwindow.cpp
index 95495b66..2119e375 100644
--- a/desktop-widgets/mainwindow.cpp
+++ b/desktop-widgets/mainwindow.cpp
@@ -88,12 +88,25 @@ MainWindow *MainWindow::m_Instance = NULL;
 
 extern "C" void showErrorFromC()
 {
+	// Show errors only if we are running in the gui thread.
+	// If we're in the gui thread, let errors accumulate.
+	if (QThread::currentThread() != QCoreApplication::instance()->thread()) {
+		return;
+	}
+
 	MainWindow *mainwindow = MainWindow::instance();
 	if (mainwindow) {
 		mainwindow->getNotificationWidget()->showNotification(get_error_string(), KMessageWidget::Error);
 	}
 }
 
+void MainWindow::showErrors()
+{
+	const char *error = get_error_string();
+	if (error && error[0]) {
+		getNotificationWidget()->showNotification(error, KMessageWidget::Error);
+	}
+}
 
 MainWindow::MainWindow() : QMainWindow(),
 	actionNextDive(0),
diff --git a/desktop-widgets/mainwindow.h b/desktop-widgets/mainwindow.h
index edd3fb0c..50590aa2 100644
--- a/desktop-widgets/mainwindow.h
+++ b/desktop-widgets/mainwindow.h
@@ -89,6 +89,10 @@ public:
 	void enableDisableCloudActions();
 	void setCheckedActionFilterTags(bool checked);
 
+	// Shows errors that have accumulated.
+	// Must be called from GUI thread.
+	void showErrors();
+
 private
 slots:
 	/* file menu action */
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Thread related crash on failed dive computer download

[Dirk Hohndel]

On Tue, Oct 31, 2017 at 08:10:32PM +0100, Berthold Stoeger wrote:
> > On Tue, Oct 31, 2017 at 03:52:28PM +0100, Berthold Stoeger wrote:
> > > As a FYI, the patch attached, though without commit-message because it is
> > > simply too ugly. It works for my particular test case, but...
> > 
> > Why is it too ugly?
> 
> Duplication of the va_args thing. At a place where it doesn't belong.

Shrug.

> Movement of C-style strings to QString back to C-style strings back to 
> QStrings. Oh my.

Yes, that always bothers me when we transition through C and C++ code and
back. We should try to keep them as just C strings for as long as
possible.

> No encapsulation whatsoever. C function defined in C++ header and declared 
> extern in C source.
> Etc. :(

Shrug. I'm trying to get a last minute fix for a bug that I introduced...

> > > + errorList.setLocalData(QStringList());
> > 
> > So we initialize to an empty list. I'm unclear if there could be messages
> > still in that local storage that haven't been delivered, yet.
> 
> You're right, the local storage should be gone once the thread exits, even 
> though the thread object is reused.
> 
> What should be cleared though is the "errors" member, because the object is 
> reused.
> 
> BTW: Another bug: the code doesn't clear the membuffer.

Oops.

> Forget the std::move - it should be removed. This would be used in standard C+
> + where list assignment makes a deep copy. A std::move on the other hand just 
> replaces pointers. But as I just learned, Qt containers do copy on write, so 
> this is not necessary. I have no idea how they would react to a std::move.

OK

> > > +// Hack! for report_error_thread()
> > > +#include "core/downloadfromdcthread.h"
> > 
> > Why is that a hack?
> 
> Because downloadfromdcthread.h should not be needed here?

This should go into a helper .h file.

> > > @@ -220,6 +220,9 @@ void DownloadFromDCWidget::updateState(states state)
> > > 
> > >   // got an error
> > >   else if (state == ERROR) {
> > >   
> > >   timer->stop();
> > > 
> > > + foreach(const QString , thread.errors) {
> > > + report_message(s.toUtf8().data());
> > > + }
> > 
> > This one I don't understand... why aren't these reported when we do the
> > std::move() above?
> 
> On completion, the thread moved the messages collected in the thread local 
> area to the "errors" member of the thread object. And here the main thread 
> displays the collected messages.
> 
> To be honest, apart from the errors mentioned above, I'm not even sure that 
> the idea behind the code is sound. If you want, I can send an updated/cleaned 
> up patch, but I have a bad feeling with this one. There probably should at 
> least be a non-varargs version.

The idea that the thread collects its messages and that the caller of the
thread then initiates their display when it is done seems sound to me.

I'd really like a version that's good enough to use in 4.7.2 - and I'm
open to further refinement once that's released.

/D
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Thread related crash on failed dive computer download

[Berthold Stoeger]

On Dienstag, 31. Oktober 2017 19:25:03 CET Dirk Hohndel wrote:

> On Tue, Oct 31, 2017 at 03:52:28PM +0100, Berthold Stoeger wrote:
> > As a FYI, the patch attached, though without commit-message because it is
> > simply too ugly. It works for my particular test case, but...
> 
> Why is it too ugly?

Duplication of the va_args thing. At a place where it doesn't belong.
Movement of C-style strings to QString back to C-style strings back to 
QStrings. Oh my.
No encapsulation whatsoever. C function defined in C++ header and declared 
extern in C source.
Etc. :(

> > +   errorList.setLocalData(QStringList());
> 
> So we initialize to an empty list. I'm unclear if there could be messages
> still in that local storage that haven't been delivered, yet.

You're right, the local storage should be gone once the thread exits, even 
though the thread object is reused.

What should be cleared though is the "errors" member, because the object is 
reused.

BTW: Another bug: the code doesn't clear the membuffer.

> > if (errorText)
> > 
> > error = str_error(errorText, internalData->devname,
> > internalData->vendor, internalData->product);> 
> > +   errors = std::move(errorList.localData());
> > +
>
> Because this seems like it would ('move'?) clear out the local storage,
> correct?

Forget the std::move - it should be removed. This would be used in standard C+
+ where list assignment makes a deep copy. A std::move on the other hand just 
replaces pointers. But as I just learned, Qt containers do copy on write, so 
this is not necessary. I have no idea how they would react to a std::move.

> > +
> > +// Dirty hack: report_error(...) crashed when not called from main
> > thread.
> > +// Thus, replace report_error(...) calls by report_error_thread(...)
> > +// calls, which collect error messages in thread local storage.
> > +extern "C" int report_error_thread(const char *fmt, ...);
> > +
> 
> Again, why is this a dirty hack?

See above.

> > +// Hack! for report_error_thread()
> > +#include "core/downloadfromdcthread.h"
> 
> Why is that a hack?

Because downloadfromdcthread.h should not be needed here?

> > @@ -220,6 +220,9 @@ void DownloadFromDCWidget::updateState(states state)
> > 
> > // got an error
> > else if (state == ERROR) {
> > 
> > timer->stop();
> > 
> > +   foreach(const QString , thread.errors) {
> > +   report_message(s.toUtf8().data());
> > +   }
> 
> This one I don't understand... why aren't these reported when we do the
> std::move() above?

On completion, the thread moved the messages collected in the thread local 
area to the "errors" member of the thread object. And here the main thread 
displays the collected messages.

To be honest, apart from the errors mentioned above, I'm not even sure that 
the idea behind the code is sound. If you want, I can send an updated/cleaned 
up patch, but I have a bad feeling with this one. There probably should at 
least be a non-varargs version.

Berthold
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Thread related crash on failed dive computer download

[Dirk Hohndel]

On Tue, Oct 31, 2017 at 03:52:28PM +0100, Berthold Stoeger wrote:
> Hi Dirk,
> 
> On Dienstag, 31. Oktober 2017 13:52:45 CET you wrote:
> > > On Oct 31, 2017, at 4:43 AM, Jan Mulder  wrote:
> > > 
> > > On 31-10-17 12:05, Jan Mulder wrote:
> > > Sounds like a fundamental problem, with the new way of error reporting ...
> > > but not sure ...
> > Yes, that makes sense. Doing the showNotification there might fail. Instead
> > we should just let the UI thread know that there is a new message to
> > display. This may prevent 4.7.2 today as I won't have time to look at this
> > until late this evening and then we might want to test it :-)
> > 
> > Unless one of you can look at it while I'm on an airplane for the next two
> > hours
> 
> After a quick look it seems that report_error(...) is used quite liberally in 
> core/qt-ble.cpp, core/downloadfromthread.cpp and core/libdivecomputer.c,
> which are all executed in DownloadThread context. Detangling this seems hard 
> as part of it is in C, other parts are callbacks from libdivecomputer, etc.

Yes, the idea was to simply just stage errors (and warnings, frankly) and
have the UI deal with them. Which we did a terrible job of in the past.
My solution made this much more responsive - and broken because of the
thread issue that I missed.

> As a quick stop-gap measure I replaced the report_error(...) calls by 
> report_error_thread(...) calls, which collect the error messages in a thread 
> local storage. The collected error messages are then passed to the main 
> thread 
> via a member.

I only had a minute to glance through the code, but it seems reasonable.

> As a FYI, the patch attached, though without commit-message because it is 
> simply too ugly. It works for my particular test case, but...

Why is it too ugly?

/D

>  void DownloadThread::run()
>  {
> + errorList.setLocalData(QStringList());

So we initialize to an empty list. I'm unclear if there could be messages
still in that local storage that haven't been delivered, yet.

>   if (errorText)
>   error = str_error(errorText, internalData->devname, 
> internalData->vendor, internalData->product);
>  
> + errors = std::move(errorList.localData());
> +

Because this seems like it would ('move'?) clear out the local storage,
correct?

> +
> +// Dirty hack: report_error(...) crashed when not called from main thread.
> +// Thus, replace report_error(...) calls by report_error_thread(...)
> +// calls, which collect error messages in thread local storage.
> +extern "C" int report_error_thread(const char *fmt, ...);
> +

Again, why is this a dirty hack?

> diff --git a/core/qt-ble.cpp b/core/qt-ble.cpp
> index a72736d4..277e44c6 100644
> --- a/core/qt-ble.cpp
> +++ b/core/qt-ble.cpp
> @@ -18,6 +18,9 @@
>  #include "core/qt-ble.h"
>  #include "core/btdiscovery.h"
>  
> +// Hack! for report_error_thread()
> +#include "core/downloadfromdcthread.h"

Why is that a hack?

> @@ -220,6 +220,9 @@ void DownloadFromDCWidget::updateState(states state)
>   // got an error
>   else if (state == ERROR) {
>   timer->stop();
> + foreach(const QString , thread.errors) {
> + report_message(s.toUtf8().data());
> + }

This one I don't understand... why aren't these reported when we do the
std::move() above?

/D
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: No user manual - again

[Pedro Neves]

On 31-10-2017 09:52, Salvador Cuñat wrote:


I'm actually building with USE_WEBENGINE=ON,  the need thing is the 
recently added tag in command line e.g. build.sh -build-with-webkit.  
This tag sets/unsets some cmake variables.




Salva:

That did the trick. Thank you very much for your help.

Cheers:

Pedro
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Root and Wayland

[Robert C. Helling]

Hi,

> Am 31.10.2017 um 12:51 schrieb Tomaz Canabrava :
> 
> 
> perhaps we should remove that, period.

A while ago, we had problems with people who at some point ran subsurface as 
root and the touched all kinds of files which meant later it did no longer run 
under the normal user. So I added the test and, since I didn’t want to for it 
users to shoot in their foot if they insisted on that introduced this command 
line option. So from that perspective it could simply go. 

The only reason I see for running with root privileges is checking if some 
problem is really because of lacking permissions. But still that’s not a good 
excuse. 

Best 
Robert
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: smtk-import

[Salvador Cuñat]

Good afternoon Pedro.

On Tue, Oct 31, 2017 at 11:00:46AM +0100, Salvador Cuñat wrote:
> Hi Pedro.
> 
> El 31/10/2017 10:44, "Pedro Neves"  escribió:
> >
> > Hi Salva:
> >
> > I'd like to update the user manual section on how to build smtk-import
> > tool, because I believe it's outdated. However, I have no clue what the
> > steps are to build it now. Any pointers?
> 
> I'm on the phone right now, but this afternoon I'll mail you with some
> instructions.
>

The script assumes you have a build tree like subsurface's usual one.
As you build subsurface regularly you already have it, but
libdivecomputer and libgit need to be installed in the dir in paralell
to the subsurface's directory.
You need to install the dependencies; in debian and ubuntu
sudo apt update && sudo apt install mdbtools-dev
should bring all of them, if not, add libglib2.0-dev to the apt
install command.
In fedora (and probably centos and friends) needed packages are named
glib2-devel and mdbtools-devel so:
dnf -y install mdbtools-devel
should be enough.
Mdbtools is, in fact, optional. If it's not installed, the script will
download and build what we need.

Move to the src directory and run
smtk2ssrf-build.sh
This will build an smtk2ssrf binary from subsurface's latest master
with the default options: Release, no commandline only, and -j4.
The binary will be located in
src/subsurface/smtk-import/build

WARNING ---> Your regular subsurface's binary has just blown away,
substituted with a bare minimum versión.  Simply rebuild subsurface as
you regularly do.

In the script's flags side, the two interesting, from a user point of
view, are  -c (--cli) and -t (--tag)

-c Builds a "just command line" version without gui capabilities. It's
the version Robert is using in his web site. Remember the full version
can run in command line mode if you pass the file names to import.

-t Forces building the provided tag, for both, the subsurface stripped
version and smtk2ssrf. E.g.
-t v4.7.1
would build subsurface library and smtk2ssrf as they were in 4.7.1
release, without  later improvements.

I think a regular user should run the script without any flags.

Please, contact me if you need further explanation on the script or
the tool.

Thank you very much, Pedro.

Salva.
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: 4.7.1 on Win7 32bit crash at startup

[Davide DB]

Sorry Lubomir, I missed your replies. The list is very active these days.

I have to check my machine. It's an Ancient Thinkpad T64 but maybe the
fastest things to do is trying the latest Subsurface daily build
tonight.

I will let you know tonight.

Thanks


On 27 October 2017 at 19:38, Lubomir I. Ivanov  wrote:
> On 27 October 2017 at 20:28, Lubomir I. Ivanov  wrote:
>>
>> other than that Subsurface works.
>> (attached image)
>>
>
> also generic GL 2.1 support works fine for this VirtualBox VM.
> so the issue i'm seeing is probably due to the fact that Qt needs a newer GL.
>
> test image attached.
>
> lubomir
> --



-- 
Davide
https://vimeo.com/bocio/videos
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: 4.7.2 - call for translations and fixes

[Lubomir I. Ivanov]

On 31 October 2017 at 18:05, Davide DB  wrote:
> I've just completed Italian translations.
>
> PS
> The new version does solve the crash on Win32 systems?
>
>

we need some help to test the win32 crashes.
a recent OpenGL related fix was added, so hopefully that fixes the GPU
related issues.

please, run "subsurface.exe -v" from the console (CMD), so that we can
gather debug output.

thanks
lubomir
--
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: 4.7.2 - call for translations and fixes

[Davide DB]

I've just completed Italian translations.

PS
The new version does solve the crash on Win32 systems?


On 31 October 2017 at 10:11, Guillaume Gardet  wrote:
> Please pull FR translation. Updated just now. :)
>
>
> Guillaume
>
>
>
>
> Le 30/10/2017 à 20:59, Dirk Hohndel a écrit :
>>
>> Hi there,
>>
>> We have a number of critical fixes in master and I'd really get 4.7.2 out.
>> Here's the current list of changes:
>>
>> - Fix potential crash when running with French localization and
>>downloading dives from a dive computer
>> - Fallback to old behavior if Facebook album creation fails
>> - Fix UI issue when adding dives to trip below
>> - Fix UI issue when closing filters
>> - Add small behavior changes to the map widget
>> - Change to a more consistent way to show errors in Subsurface
>> - Solve several potential issues with the cloud account authentication
>>flow
>> - Fix a potential problem when saving data that was loaded from a git
>>repository into cloud storage
>> - Fix error where newly created cloud accounts failed to create
>>the remote repository on Windows
>> - Detect and try to work around the fact that the map module requires
>>at least OpenGL 2.1 (previously Subsurface would crash when OpenGL
>>was too old - often on old Win32 versions)
>>
>> Is there anything else that one of you has "almost ready to go" and that I
>> should wait for? In this process here we fixed a few translations and also
>> added a couple of strings that were missing before. It should only be a
>> few strings for most languages - I'd appreciate if people could check.
>>
>> If nothing comes up we can turn it into a Halloween version tomorrow :-)
>>
>> Thanks
>>
>> /D
>> ___
>> subsurface mailing list
>> subsurface@subsurface-divelog.org
>> http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
>
>
> ___
> subsurface mailing list
> subsurface@subsurface-divelog.org
> http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface



-- 
Davide
https://vimeo.com/bocio/videos
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Thread related crash on failed dive computer download

[Berthold Stoeger]

Hi Dirk,

On Dienstag, 31. Oktober 2017 13:52:45 CET you wrote:
> > On Oct 31, 2017, at 4:43 AM, Jan Mulder  wrote:
> > 
> > On 31-10-17 12:05, Jan Mulder wrote:
> > Sounds like a fundamental problem, with the new way of error reporting ...
> > but not sure ...
> Yes, that makes sense. Doing the showNotification there might fail. Instead
> we should just let the UI thread know that there is a new message to
> display. This may prevent 4.7.2 today as I won't have time to look at this
> until late this evening and then we might want to test it :-)
> 
> Unless one of you can look at it while I'm on an airplane for the next two
> hours

After a quick look it seems that report_error(...) is used quite liberally in 
core/qt-ble.cpp, core/downloadfromthread.cpp and core/libdivecomputer.c,
which are all executed in DownloadThread context. Detangling this seems hard 
as part of it is in C, other parts are callbacks from libdivecomputer, etc.

As a quick stop-gap measure I replaced the report_error(...) calls by 
report_error_thread(...) calls, which collect the error messages in a thread 
local storage. The collected error messages are then passed to the main thread 
via a member.

As a FYI, the patch attached, though without commit-message because it is 
simply too ugly. It works for my particular test case, but...

Berthold

diff --git a/core/downloadfromdcthread.cpp b/core/downloadfromdcthread.cpp
index e01de288..3ce4d7ab 100644
--- a/core/downloadfromdcthread.cpp
+++ b/core/downloadfromdcthread.cpp
@@ -1,8 +1,10 @@
 #include "downloadfromdcthread.h"
 #include "core/libdivecomputer.h"
+#include "core/membuffer.h"
 #include "core/subsurface-qt/SettingsObjectWrapper.h"
 #include 
 #include 
+#include 
 
 QStringList vendorList;
 QHash productList;
@@ -10,6 +12,24 @@ static QHash mobileProductList;	// BT, BLE or FTDI support
 QMap descriptorLookup;
 ConnectionListModel connectionListModel;
 
+QThreadStorage errorList;
+
+#define VA_BUF(b, fmt) do { va_list args; va_start(args, fmt); put_vformat(b, fmt, args); va_end(args); } while (0)
+
+extern "C"
+int report_error_thread(const char *fmt, ...)
+{
+	struct membuffer buf = { 0 };
+
+	/* Previous unprinted errors? Add a newline in between */
+	VA_BUF(, fmt);
+	mb_cstring();
+
+	if(buf.len)
+		errorList.localData().append(QString(buf.buffer));
+	return -1;
+}
+
 static QString str_error(const char *fmt, ...)
 {
 	va_list args;
@@ -27,6 +47,7 @@ DownloadThread::DownloadThread()
 
 void DownloadThread::run()
 {
+	errorList.setLocalData(QStringList());
 	auto internalData = m_data->internalData();
 	internalData->descriptor = descriptorLookup[m_data->vendor() + m_data->product()];
 	internalData->download_table = 	
@@ -51,6 +72,8 @@ void DownloadThread::run()
 	if (errorText)
 		error = str_error(errorText, internalData->devname, internalData->vendor, internalData->product);
 
+	errors = std::move(errorList.localData());
+
 	qDebug() << "Finishing the thread" << errorText << "dives downloaded" << downloadTable.nr;
 	auto dcs = SettingsObjectWrapper::instance()->dive_computer_settings;
 	dcs->setVendor(internalData->vendor);
diff --git a/core/downloadfromdcthread.h b/core/downloadfromdcthread.h
index ff4b8f39..2dd134ff 100644
--- a/core/downloadfromdcthread.h
+++ b/core/downloadfromdcthread.h
@@ -79,6 +79,7 @@ public:
 
 	Q_INVOKABLE DCDeviceData *data();
 	QString error;
+	QStringList errors;
 
 private:
 	DCDeviceData *m_data;
@@ -101,4 +102,10 @@ extern QStringList vendorList;
 extern QHash productList;
 extern QMap descriptorLookup;
 extern ConnectionListModel connectionListModel;
+
+// Dirty hack: report_error(...) crashed when not called from main thread.
+// Thus, replace report_error(...) calls by report_error_thread(...)
+// calls, which collect error messages in thread local storage.
+extern "C" int report_error_thread(const char *fmt, ...);
+
 #endif
diff --git a/core/libdivecomputer.c b/core/libdivecomputer.c
index d8fe54e8..86456751 100644
--- a/core/libdivecomputer.c
+++ b/core/libdivecomputer.c
@@ -18,6 +18,8 @@
 #include "libdivecomputer.h"
 #include "core/version.h"
 
+int report_error_thread(const char *fmt, ...);
+
 #if !defined(SSRF_LIBDC_VERSION) || SSRF_LIBDC_VERSION < 2
 #pragma message "Subsurface requires a reasonably current version of the Subsurface-branch"
 #pragma message "of libdivecomputer (at least version 2 of our API)."
@@ -106,10 +108,10 @@ static int parse_gasmixes(device_data_t *devdata, struct dive *dive, dc_parser_t
 	if (rc == DC_STATUS_SUCCESS) {
 		if (ntanks && ntanks < ngases) {
 			shown_warning = true;
-			report_error("Warning: different number of gases (%d) and cylinders (%d)", ngases, ntanks);
+			report_error_thread("Warning: different number of gases (%d) and cylinders (%d)", ngases, ntanks);
 		} else if (ntanks > ngases) {
 			shown_warning = true;
-			report_error("Warning: 

Re: Thread related crash on failed dive computer download

[Dirk Hohndel]

> On Oct 31, 2017, at 4:43 AM, Jan Mulder  wrote:
> 
> On 31-10-17 12:05, Jan Mulder wrote:
> 
>> But Dirk has to worry, as it suspiciously close to the new error handling 
>> code that has just landed in master.
> 
> for Dirk:
> 
> from Qt 5.9.2 from build from source with debug:
> 
> failed to connect to the controller  00:80:25:4A:0F:C2 with error ""
> QObject: Cannot create children for a parent that is in a different thread.
> (Parent is QLabel(0x562909c0), parent's thread is 
> QThread(0x55ea2cc0), current thread is DownloadThread(0x7fffcbc0)
> ASSERT failure in QCoreApplication::sendEvent: "Cannot send events to objects 
> owned by a different thread. Current thread 0x0x7fffcbc0. Receiver '' (of 
> type 'QToolButton') was created in thread 0x0x55ea2cc0", file 
> kernel/qcoreapplication.cpp, line 563
> 
> Sounds like a fundamental problem, with the new way of error reporting ... 
> but not sure ...

Yes, that makes sense. Doing the showNotification there might fail. Instead we 
should just let the UI thread know that there is a new message to display. This 
may prevent 4.7.2 today as I won't have time to look at this until late this 
evening and then we might want to test it :-)

Unless one of you can look at it while I'm on an airplane for the next two hours

/D
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Import data from Scubapro Aladin Sport Matrix

[Jef Driesen]

On 2017-10-31 12:12, Berthold Stoeger wrote:

Hi Jef,

On Dienstag, 31. Oktober 2017 11:29:00 CET Jef Driesen wrote:

On 2017-10-23 00:55, Berthold Stoeger wrote:
> On Sonntag, 22. Oktober 2017 04:31:39 CEST Linus Torvalds wrote:
>> so you literally *could* try to claim it's a G2 and try to see how far
>> it downloads.
>
> The handshake is different. The G2 client sends 1b and 1c1027 and
> expects
> 01 as the answer twice. The Aladin Sport returns 00 in both cases.
> Since
> LogTrak doesn't do any handshake, I just removed the call to
> scubapro_g2_handshake().

Am I right you only tested downloading over bluetooth (BLE)? Can you
also try over USB? I suspect that logtrak still does the handshaking
when downloading over USB. If that's correct, then omitting the
handshaking is not specific to the Sport Matrix, but the underlying
transport.


To my knowledge the Sport Matrix does only support BLE! At least that's 
how I

read
https://ww2.scubapro.com/en-GB/SWE/instruments/computers/products/aladin-sport-matrix.aspx:

| For PCs and Macs that are not equipped with built-in Bluetooth Low 
Energy
| technology, the end user will have to buy a Bluetooth Low Energy USB 
dongle

| which is available on the market.


I simply assumed it was similar to the G2, which supports both.

That's why I feel that dc_descriptor_get_transport() should return 
flags, not

enums. You want to differentiate between USB, BT, USB+BT, don't you?


Indeed! Such a change is already on my todo list :-)

Because it breaks backwards compatibility, I will introduce it together 
with some other non-backwards compatible changes, like the I/O 
factoring. That way applications only have to be updated once.



But yes, doing handshakes based on transport not model is a good idea.


The less "if (model == X)" code we need, the better!

Note that in most cases the exact model number is communicated by the 
device during the download. Needing the model number up front breaks 
this auto-detection mechanism. (You would be surprised how many users 
don't know exactly which model they have. For some dive computers, the 
naming can indeed be confusing.)


I don't have access to the computer right know and will answer to your 
other

remarks later.


Excellent!

Jef
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Root and Wayland

[Tomaz Canabrava]

On Tue, Oct 31, 2017 at 11:56 AM, Miika Turkia 
wrote:

> On Tue, Oct 31, 2017 at 12:31 PM, Martin Měřinský 
> wrote:
> > Hello.
> > These days more and more distributions are using Wayland. Last Ubuntu
> > e.g. AFAIK Wayland design feature is that UI applications are not
> > allowed to run as root.
> >
> > From Subsurface manual: "If you are not root, you may not be a member
> > of that group and won’t be able to use the USB port." Well, if you are
> > root, you cannot run Subsurface under Wayland at all(?) Fortunately
> > it's not necessary, but output doesn't look good.
>
> We do not recommend running Subsurface as root and try to get users to
> configure their system so that root is not needed (i.e. adding
> themselves to the dialout group). That is the whole purpose of the
> specific chapter in user manual. But this sentence should be reworded
> to make it clear that user should set the groups properly and not run
> Subsurface as root.
>
> > I would humbly suggest to detect Wayland and root privileges, show
> > better command line message and quit.
> > Or fix it somehow like GParted 0.30.0 did?
> > Maybe we should mention it in the manual.
>
> There should not be any reason to run Subsurface as root, no matter
> whether Wayland or not. So any attempt to run with root privileges
> should result in an error message. But in Wayland's case it might be
> better not to mention the allow_run_as_root option at all (or tell the
> xhost cmdline).
>
> > Also --allow_run_as_root option is not mentioned in subsurface --help
> > output nor in the manual.
>
>
perhaps we should remove that, period.

I am not sure if this should be documented or not. Running software as
> root is generally a bad idea, so documenting how to do that sounds a
> bit contradictory to me.
>
> miika
> ___
> subsurface mailing list
> subsurface@subsurface-divelog.org
> http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
>
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Thread related crash on failed dive computer download

[Jan Mulder]

On 31-10-17 12:05, Jan Mulder wrote:

But Dirk has to worry, as it suspiciously close to the new error 
handling code that has just landed in master.


for Dirk:

from Qt 5.9.2 from build from source with debug:

failed to connect to the controller  00:80:25:4A:0F:C2 with error ""
QObject: Cannot create children for a parent that is in a different thread.
(Parent is QLabel(0x562909c0), parent's thread is 
QThread(0x55ea2cc0), current thread is DownloadThread(0x7fffcbc0)
ASSERT failure in QCoreApplication::sendEvent: "Cannot send events to 
objects owned by a different thread. Current thread 0x0x7fffcbc0. 
Receiver '' (of type 'QToolButton') was created in thread 
0x0x55ea2cc0", file kernel/qcoreapplication.cpp, line 563


Sounds like a fundamental problem, with the new way of error reporting 
... but not sure ...


As compiled with debug it trips on the assert here.

--jan


___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Import data from Scubapro Aladin Sport Matrix

[Berthold Stoeger]

Hi Jef,

On Dienstag, 31. Oktober 2017 11:29:00 CET Jef Driesen wrote:
> On 2017-10-23 00:55, Berthold Stoeger wrote:
> > On Sonntag, 22. Oktober 2017 04:31:39 CEST Linus Torvalds wrote:
> >> so you literally *could* try to claim it's a G2 and try to see how far
> >> it downloads.
> > 
> > The handshake is different. The G2 client sends 1b and 1c1027 and
> > expects
> > 01 as the answer twice. The Aladin Sport returns 00 in both cases.
> > Since
> > LogTrak doesn't do any handshake, I just removed the call to
> > scubapro_g2_handshake().
> 
> Am I right you only tested downloading over bluetooth (BLE)? Can you
> also try over USB? I suspect that logtrak still does the handshaking
> when downloading over USB. If that's correct, then omitting the
> handshaking is not specific to the Sport Matrix, but the underlying
> transport.

To my knowledge the Sport Matrix does only support BLE! At least that's how I 
read 
https://ww2.scubapro.com/en-GB/SWE/instruments/computers/products/aladin-sport-matrix.aspx:

| For PCs and Macs that are not equipped with built-in Bluetooth Low Energy
| technology, the end user will have to buy a Bluetooth Low Energy USB dongle
| which is available on the market.

That's why I feel that dc_descriptor_get_transport() should return flags, not 
enums. You want to differentiate between USB, BT, USB+BT, don't you?

But yes, doing handshakes based on transport not model is a good idea.

I don't have access to the computer right know and will answer to your other 
remarks later.

Thanks,

Berthold



___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Thread related crash on failed dive computer download

[Jan Mulder]

Berthold,

On 31-10-17 11:53, Berthold Stoeger wrote:

Dear all,

I tried to reset my git repository to head and now I'm getting a crash which I
didn't get before. I tried "git clean -fx", but that didn't help.

The crash happens when using invalid Bluetooth addresses. Call trace:
- core/qt-ble.c l.326: report_error(...)
- core/errorhelper.c l.43: error_cb()
- desktop-widgets/mainwindow.cpp l.93: showNotification(...)

This call doesn't return but produces the following output:


The crash probably has nothing to do with the membuffer code (which itself
looks scarily thread-unsafe):
Replacing l. 93 of desktop-widgets/mainwindow.cpp by
   mainwindow->getNotificationWidget()->showNotification("test",...);
still produces the crash. But
   mainwindow->getNotificationWidget()->showNotification("",...);
does not crash.

I have a feeling that I'm doing something obviously wrong, but I don't see it.
Thank you,


Don't worry. It is not your build environment as I can easily reproduce 
your crash. BLE with incorrect MAC address.


But Dirk has to worry, as it suspiciously close to the new error 
handling code that has just landed in master.


--jan

___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Root and Wayland

[Miika Turkia]

On Tue, Oct 31, 2017 at 12:31 PM, Martin Měřinský  wrote:
> Hello.
> These days more and more distributions are using Wayland. Last Ubuntu
> e.g. AFAIK Wayland design feature is that UI applications are not
> allowed to run as root.
>
> From Subsurface manual: "If you are not root, you may not be a member
> of that group and won’t be able to use the USB port." Well, if you are
> root, you cannot run Subsurface under Wayland at all(?) Fortunately
> it's not necessary, but output doesn't look good.

We do not recommend running Subsurface as root and try to get users to
configure their system so that root is not needed (i.e. adding
themselves to the dialout group). That is the whole purpose of the
specific chapter in user manual. But this sentence should be reworded
to make it clear that user should set the groups properly and not run
Subsurface as root.

> I would humbly suggest to detect Wayland and root privileges, show
> better command line message and quit.
> Or fix it somehow like GParted 0.30.0 did?
> Maybe we should mention it in the manual.

There should not be any reason to run Subsurface as root, no matter
whether Wayland or not. So any attempt to run with root privileges
should result in an error message. But in Wayland's case it might be
better not to mention the allow_run_as_root option at all (or tell the
xhost cmdline).

> Also --allow_run_as_root option is not mentioned in subsurface --help
> output nor in the manual.

I am not sure if this should be documented or not. Running software as
root is generally a bad idea, so documenting how to do that sounds a
bit contradictory to me.

miika
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Thread related crash on failed dive computer download

[Berthold Stoeger]

Dear all,

I tried to reset my git repository to head and now I'm getting a crash which I 
didn't get before. I tried "git clean -fx", but that didn't help. 

The crash happens when using invalid Bluetooth addresses. Call trace:
- core/qt-ble.c l.326: report_error(...)
- core/errorhelper.c l.43: error_cb()
- desktop-widgets/mainwindow.cpp l.93: showNotification(...)

This call doesn't return but produces the following output:

*
QObject: Cannot create children for a parent that is in a different thread.
(Parent is QLabel(0x55a2edc69d70), parent's thread is QThread(0x55a2ed98bf50), 
current thread is DownloadThread(0x7ffc7ab20f70)
QObject::killTimer: Timers cannot be stopped from another thread
QObject::startTimer: Timers cannot be started from another thread
[...last two lines repeated numerous times...]
QBasicTimer::start: Timers cannot be started from another thread
QObject::killTimer: Timers cannot be stopped from another thread
QObject::startTimer: Timers cannot be started from another thread
QObject::killTimer: Timers cannot be stopped from another thread
QObject::startTimer: Timers cannot be started from another thread
QObject::killTimer: Timers cannot be stopped from another thread
QObject::startTimer: Timers cannot be started from another thread
QObject::killTimer: Timers cannot be stopped from another thread
QObject::startTimer: Timers cannot be started from another thread
Cannot make QOpenGLContext current in a different thread
Aborted
*

The crash probably has nothing to do with the membuffer code (which itself 
looks scarily thread-unsafe):
Replacing l. 93 of desktop-widgets/mainwindow.cpp by
  mainwindow->getNotificationWidget()->showNotification("test",...);
still produces the crash. But
  mainwindow->getNotificationWidget()->showNotification("",...);
does not crash.

I have a feeling that I'm doing something obviously wrong, but I don't see it.
Thank you,

Berthold
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Root and Wayland

[Martin Měřinský]

Hello.
These days more and more distributions are using Wayland. Last Ubuntu
e.g. AFAIK Wayland design feature is that UI applications are not
allowed to run as root.

From Subsurface manual: "If you are not root, you may not be a member
of that group and won’t be able to use the USB port." Well, if you are
root, you cannot run Subsurface under Wayland at all(?) Fortunately
it's not necessary, but output doesn't look good.

/usr/bin/subsurface --version
Subsurface v4.7.1, built with libdivecomputer v0.6.0-devel-Subsurface-
branch (0099aeeb7088c7ff3c4513f100b1cb5516f48233)

sudo /usr/bin/subsurface
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-
root'
No protocol specified
QXcbConnection: Could not connect to display :0
Aborted

sudo /usr/bin/subsurface --allow_run_as_root
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-
root'
No protocol specified
QXcbConnection: Could not connect to display :0
Aborted

xhost +si:localuser:root
sudo /usr/bin/subsurface
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-
root'
You are running Subsurface as root. This is not recommended.
If you insist to do so, run with option --allow_run_as_root.

xhost +si:localuser:root
sudo /usr/bin/subsurface --allow_run_as_root
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-
root'
Runs OK.

I would humbly suggest to detect Wayland and root privileges, show
better command line message and quit.
Or fix it somehow like GParted 0.30.0 did?
Maybe we should mention it in the manual.

Also --allow_run_as_root option is not mentioned in subsurface --help
output nor in the manual.

Thanks in advace
Martin M.
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Import data from Scubapro Aladin Sport Matrix

[Jef Driesen]

On 2017-10-23 00:55, Berthold Stoeger wrote:

On Sonntag, 22. Oktober 2017 04:31:39 CEST Linus Torvalds wrote:


so you literally *could* try to claim it's a G2 and try to see how far
it downloads.


The handshake is different. The G2 client sends 1b and 1c1027 and 
expects
01 as the answer twice. The Aladin Sport returns 00 in both cases. 
Since

LogTrak doesn't do any handshake, I just removed the call to
scubapro_g2_handshake().


Am I right you only tested downloading over bluetooth (BLE)? Can you 
also try over USB? I suspect that logtrak still does the handshaking 
when downloading over USB. If that's correct, then omitting the 
handshaking is not specific to the Sport Matrix, but the underlying 
transport.


Note that logtrak also supports a second variant of the handshaking 
(0xF4), in case the other two (0x1B and 0x1C) return zero, but I have 
never seen that so far. Maybe the Sport Matrix is in this category?


Apart from that - you were right - it seems to be compatible with the 
G2! Only
the dates are incorrect - I will try to figure out what is going on. 
All I had
to do was adding a case in the switch in uwatec_smart_parser_create(). 
The id
of the Aladin Sport Matrix is 0xa5, by the way. And it works! Thank you 
very

much for your support!


I believe you have the model number wrong. I think it should be 0x17, 
and your test seems to confirm that:


Command: 0110
Output : 0117

Do you mind sending me a libdivecomputer memory dump and logfile?

Jef
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: No user manual - again

[Pedro Neves]

On 30-10-2017 23:19, Dirk Hohndel wrote:

You want USE_WEBENGINE=OFF



Dirk:

I've changed this option on ccmake. Still no luck. Each time a build 
finishes, NO_PRINTING and NO_USERMANUAL revert back to ON...

I'll try later on with a fresh src...

Cheers:

Pedro
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

smtk-import

[Pedro Neves]

Hi Salva:

I'd like to update the user manual section on how to build smtk-import 
tool, because I believe it's outdated. However, I have no clue what the 
steps are to build it now. Any pointers?


Thanks in advance...

Pedro
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: 4.7.2 - call for translations and fixes

[Guillaume Gardet]

Please pull FR translation. Updated just now. :)


Guillaume



Le 30/10/2017 à 20:59, Dirk Hohndel a écrit :

Hi there,

We have a number of critical fixes in master and I'd really get 4.7.2 out.
Here's the current list of changes:

- Fix potential crash when running with French localization and
   downloading dives from a dive computer
- Fallback to old behavior if Facebook album creation fails
- Fix UI issue when adding dives to trip below
- Fix UI issue when closing filters
- Add small behavior changes to the map widget
- Change to a more consistent way to show errors in Subsurface
- Solve several potential issues with the cloud account authentication
   flow
- Fix a potential problem when saving data that was loaded from a git
   repository into cloud storage
- Fix error where newly created cloud accounts failed to create
   the remote repository on Windows
- Detect and try to work around the fact that the map module requires
   at least OpenGL 2.1 (previously Subsurface would crash when OpenGL
   was too old - often on old Win32 versions)

Is there anything else that one of you has "almost ready to go" and that I
should wait for? In this process here we fixed a few translations and also
added a couple of strings that were missing before. It should only be a
few strings for most languages - I'd appreciate if people could check.

If nothing comes up we can turn it into a Halloween version tomorrow :-)

Thanks

/D
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface


___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface