Open / Save to cloud on Windows 10

Thu Feb 23 04:31:20 PST 2017

Hi guys,

Pull request sent (https://github.com/Subsurface-divelog/subsurface/pull/222
)
Sorry for the late reply, these days were quite busy with a bit of coding
but mainly with my Instructor Training Course :)

Based on your recommendations I've added a subsurface_stat portability
function. There I used the corresponding widechar implementation wstat()
for windows (after utf8 to utf16 conversion) and simply called the normal
stat() otherwise. It seems to work nicely.

>From what I understand when reading msdn, stat() is actually expecting a
path encoded with the codepoint that is defined in system settings so it
can be many things but rarely utf8... Win32 only apps do not face issues
since they would read this from user input or other win32 api that are
already returning paths/stings encoded with that codepoint. In our case
everything is converted to utf8 before being returned to the app level. At
least this is my current understanding of the issue we faced.

Since I like to work with tests, I made some little changes to the testing
code in order to be able to run cross compiled tests under windows too. And
added a case that test specificaly our issue.
The changes I've made there are not yet providing a fully flavored solution
(but it is a step forward, that allowed me to test without too much
troubles).

What could come next (but I prefer to do this in a separate change) would
be:
- Packaging of the tests together with dependencies and test data for
windows run
 (currently I went the hard way of manually copying what I needed)
- Making test execution not depending on user name. (All cloud part of
TestGitStorage implementation uses default path based on user dir to
execute tests.)
- Make test data user independent. (When I run the tests I get many:
"didn't find picture entry for
/home/hohndel/subsurface/dives/images/PA102039.jpg"
caused by hard coded path in the test cases)
- I still could not run TestPreferences.exe I get a strange missing
dependency message (certainly explained by my manual copy of the
dependencies ;))
- Some issue with recently added TestMerge and TestParse caused by a
trailing \r in the value from written file
   Actual   (readin.takeFirst()) : "<divelog program='subsurface'
version='3'>"
   Expected (written.takeFirst()): "<divelog program='subsurface'
version='3'>\r"

For now I got TestGitStorage to:
1. Reproduce the issue when the fix was not there. see log below, I cut off
the missing image messages
2. Pass once the fix is included. see log below too

Have a look at the pull request, I will be happy to make any improvements
or fixes to the change. (I will not be unhappy if you pull it in as is)

Cheers,

Jeremie

Before:
********* Start testing of TestGitStorage *********
Config: Using QtTest library 5.7.0, Qt 5.7.0 (i386-little_endian-ilp32
shared (dynamic) release build; by GCC 4.9.4)
PASS   : TestGitStorage::initTestCase()
PASS   : TestGitStorage::testSetup()
PASS   : TestGitStorage::testGitStorageLocal(ASCII path)
FAIL!  : TestGitStorage::testGitStorageLocal(Non ASCII path) Compared
values are not the same
   Actual   (save_dives(qUtf8Printable(testDirName + "[test]"))): -1
   Expected (0)                                                 : 0
/home/jeremie/dev/subsurface-win/subsurface/tests/testgitstorage.cpp(81) :
failure location
FAIL!  : TestGitStorage::testGitStorageCloud() Compared values are not the
same
   Actual   (parse_file(qUtf8Printable(cloudTestRepo))): -1
   Expected (0)                                        : 0
/home/jeremie/dev/subsurface-win/subsurface/tests/testgitstorage.cpp(107) :
failure location
FAIL!  : TestGitStorage::testGitStorageCloudOfflineSync() Compared values
are not the same
   Actual   (parse_file(qUtf8Printable(localCacheRepo))): -1
   Expected (0)                                         : 0
/home/jeremie/dev/subsurface-win/subsurface/tests/testgitstorage.cpp(129) :
failure location
FAIL!  : TestGitStorage::testGitStorageCloudMerge() Compared values are not
the same
   Actual   (parse_file(qUtf8Printable(localCacheRepoSave))): -1
   Expected (0)                                             : 0
/home/jeremie/dev/subsurface-win/subsurface/tests/testgitstorage.cpp(179) :
failure location
FAIL!  : TestGitStorage::testGitStorageCloudMerge2() Compared values are
not the same
   Actual   (parse_file(qUtf8Printable(localCacheRepo))): -1
   Expected (0)                                         : 0
/home/jeremie/dev/subsurface-win/subsurface/tests/testgitstorage.cpp(227) :
failure location
FAIL!  : TestGitStorage::testGitStorageCloudMerge3() Compared values are
not the same
   Actual   (parse_file(qUtf8Printable(cloudTestRepo))): -1
   Expected (0)                                        : 0
/home/jeremie/dev/subsurface-win/subsurface/tests/testgitstorage.cpp(284) :
failure location
PASS   : TestGitStorage::cleanupTestCase()
Totals: 4 passed, 6 failed, 0 skipped, 0 blacklisted, 20258ms
********* Finished testing of TestGitStorage *********

After:
********* Start testing of TestGitStorage *********
Config: Using QtTest library 5.7.0, Qt 5.7.0 (i386-little_endian-ilp32
shared (dynamic) release build; by GCC 4.9.4)
PASS   : TestGitStorage::initTestCase()
PASS   : TestGitStorage::testSetup()
PASS   : TestGitStorage::testGitStorageLocal(ASCII path)
PASS   : TestGitStorage::testGitStorageLocal(Non ASCII path)
PASS   : TestGitStorage::testGitStorageCloud()
PASS   : TestGitStorage::testGitStorageCloudOfflineSync()
PASS   : TestGitStorage::testGitStorageCloudMerge()
PASS   : TestGitStorage::testGitStorageCloudMerge2()
PASS   : TestGitStorage::testGitStorageCloudMerge3()
PASS   : TestGitStorage::cleanupTestCase()
Totals: 10 passed, 0 failed, 0 skipped, 0 blacklisted, 125181ms
********* Finished testing of TestGitStorage *********

2017-02-21 0:47 GMT+07:00 Linus Torvalds <torvalds at linux-foundation.org>:

> On Mon, Feb 20, 2017 at 9:41 AM, Dirk Hohndel <dirk at hohndel.org> wrote:
> >
> > How about writing a wrapper function that can be called from C in
> > qthelper.cpp (we have quite a few of them there already) and have that do
> > something like
> >
> > extern "C" bool dir_exists(const char *dir)
> > {
> >         return QDir(dir).exists();
> > }
>
> So we do actually want more than to check if it's a directory. We use
> "stat()" for two things in the git wrapper:
>
>  - to check the existing cache:
>
>     if it's a directorty, we uupdate it
>
>  - if it's a non-directory,. we do warnings/cleanup
>
>  - if it doesn't exist, we may create it.
>
> So that code actually wants at least a ternary return value, or just a
> regular stat() like it does now.
>
> Then in is_git_repository() we do indeed just want to know "does this
> directory exist".
>
> We *could* just use our existing "subsurface_open() + fstat()", which
> is already a pattern we use, but doing "open()" on a directory ends up
> being one of those things I could easily see not working under Windows
> either.
>
>                      Linus
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20170223/012fcd4d/attachment.html>