Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port FreeDV to wxWidgets 3.1 #68

Merged
merged 7 commits into from
Aug 8, 2020
Merged

Port FreeDV to wxWidgets 3.1 #68

merged 7 commits into from
Aug 8, 2020

Conversation

tmiw
Copy link
Collaborator

@tmiw tmiw commented Aug 5, 2020

Updates FreeDV UI code to support wxWidgets 3.1. Resolves issues #67 and #27.

@drowe67, this branch seems to look fine on Mac but it wouldn't hurt to test on Windows and Linux too.

Test Plan

Try all Dialogs and basic functionality (say decoding a test file from wav dir) on:

Test OS wxWidgets Who Pass
1 OSX 3.0 Mooneer Pass
1 OSX 3.1 Mooneer Pass
2 Linux 3.0 David Pass
3 Linux 3.1 David Pass
4 WIndows 3.0 not tested -
5 Windows 3.1 not tested -

Windows could be 32 or 64 bit.

@@ -48,17 +48,17 @@ typedef struct {
//-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
// Class AuiNotebookNoKbd
//-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
class AuiNotebookNoKbd : public wxAuiNotebook
class AuiNotebookNoKbd : public wxNotebook
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to move from wxAuiNotebook to wxNotebook due to a stack overflow bug when creating instances of this object. Maybe wx 3.1.5 or later will resolve?

@drowe67
Copy link
Owner

drowe67 commented Aug 5, 2020

@tmiw wow that's a big effort - thanks 😄 I haven't had to play with all the wx_align commands for a while - last time I did I found it very confusing. Many of these screens were generated by some sort of layout tool in 2012, I have tried to maintain them manually since.

Yes we will need to test on Windows and Linux, and that the changes still work OK with wxWidgets 3.0. I guess we know (via Travis), that they build OK on 3.0 👍

I can see in CMakeList.txt there is an option to download a wxWidgets tarball. Can you pls post the cmake command line you used to configure for wxWidgets 3.1?

@drowe67
Copy link
Owner

drowe67 commented Aug 5, 2020

@ra1nb0w can you pls try this PR?

@tmiw
Copy link
Collaborator Author

tmiw commented Aug 5, 2020

@drowe67, the version of wxWidgets that the build is using is hardcoded into cmake/BuildWxWidgets.cmake. I used the same commands that are in build_osx.sh, namely:

# Finally, build freedv-gui
cd $FREEDVGUIDIR && git pull
mkdir  -p build_osx && cd build_osx && rm -Rf *
cmake -DCMAKE_BUILD_TYPE=Debug  -DBOOTSTRAP_WXWIDGETS=1 -DUSE_STATIC_DEPS=1 -DUSE_STATIC_SPEEXDSP=1 -DHAMLIB_INCLUDE_DIR=${HAMLIBDIR}/include -DHAMLIB_LIBRARY=${HAMLIBDIR}/lib/libhamlib.a -DCODEC2_BUILD_DIR=$CODEC2DIR/build_osx -DLPCNET_BUILD_DIR=$LPCNETDIR/build_osx ..
make VERBOSE=1

# Rebuild now that wxWidgets is bootstrapped
cmake -DCMAKE_BUILD_TYPE=Debug  -DBOOTSTRAP_WXWIDGETS=1 -DUSE_STATIC_DEPS=1 -DUSE_STATIC_SPEEXDSP=1 -DHAMLIB_INCLUDE_DIR=${HAMLIBDIR}/include -DHAMLIB_LIBRARY=${HAMLIBDIR}/lib/libhamlib.a -DCODEC2_BUILD_DIR=$CODEC2DIR/build_osx -DLPCNET_BUILD_DIR=$LPCNETDIR/build_osx ..
make VERBOSE=1

The above causes CMake to download and build wxWidgets itself.

@ra1nb0w
Copy link
Contributor

ra1nb0w commented Aug 6, 2020

Fix issue #67 but not #27. See the image:
untitled
it is interesting that when you use the screenshot tool the window starts to be correct. Maybe some redraw signal?
Anyway, thank you @tmiw!

@drowe67 when you make a release (bb9558c) you should tag it; in that way I can make a release on macports and you get the release on github releases.

@tmiw
Copy link
Collaborator Author

tmiw commented Aug 6, 2020

@ra1nb0w, what version of macOS are you using? On 10.15.5 I see the following (same live vs. screenshot):

Light mode:
Screen Shot 2020-08-05 at 11 33 45 PM

Dark mode:
Screen Shot 2020-08-05 at 11 33 57 PM

The UI also seems to dynamically change as expected when I change modes in System Preferences->General.

@ra1nb0w
Copy link
Contributor

ra1nb0w commented Aug 6, 2020

Sorry, I was too concise.
Setup:
macOS 10.15.6 19G73
Xcode 11.5 11E608c
wxWidgets-3.2 @3.1.4 (from macports)
freedv-gui at commit bb9558c

the image that I attached was at the end of the startup without touching anything. If I move around the window after a while it becomes clean but if I open a config panel then the main window becomes dirty again. Always without starting the capture and in dark mode; with "white mode" everything works correctly. if you prefer I can take a short video.

@tmiw
Copy link
Collaborator Author

tmiw commented Aug 6, 2020

@ra1nb0w, OK, I was able to duplicate it on mine too and just submitted a commit to fix that particular bug. Let me know if that helps.

@ra1nb0w
Copy link
Contributor

ra1nb0w commented Aug 6, 2020

fixed! thank you @tmiw!
When @drowe67 will tag the new release I update the macports freedv with this patch.

@drowe67
Copy link
Owner

drowe67 commented Aug 6, 2020

Nice work everyone! I've added a test plan at the top of this PR. I think you guys may already have tried out the OSX cases, if so pls fill in your results.

We'll have to figure out how to modify the freedv-gui/docker scripts to bootstrap wxWidgets 3.1, it currently uses the Fedora 3.0 package.

@tmiw
Copy link
Collaborator Author

tmiw commented Aug 7, 2020

Considering that macOS dark mode support didn't seem to get added to wxWidgets until v3.1.3, I'd not recommend running FreeDV with v3.0 on newer versions of the OS. It should still be okay for older versions without dark mode, however. I'll see about building a binary with MacPorts' wxWidgets to test 3.0.

For Docker, we could just have the build download/build its own wxWidgets like what build_osx.sh does for Mac. The downside of course is that it'll overwrite whatever's already installed on the user's machine (or will dramatically increase the size of the freedv binary).

@tmiw
Copy link
Collaborator Author

tmiw commented Aug 7, 2020

@drowe67, I was able to build with MacPorts' wxWidgets (v3.0.4) and it looks like dark mode definitely doesn't behave well with freedv-gui. However, the app itself looks okay in the default "light" mode. Perhaps we can just enforce 3.1 with CMake when building on Mac unless one is building on an old enough version of the OS?

@ra1nb0w
Copy link
Contributor

ra1nb0w commented Aug 7, 2020

I am maintaining freedv-gui on macports (portfile) therefore you can use it as example.
I went back to wxwidgets-3.0 to avoid bug #67.
To avoid bug #27 I use

<key>NSRequiresAquaSystemAppearance</key>
<true/>

When 1.4.2 will be tagged I apply this patch and I move back to wxwidgets-3.2 and enable dark mode (version 3.1.4 contains many fixes and enhancement for macOS).

@drowe67
Copy link
Owner

drowe67 commented Aug 7, 2020

@drowe67, I was able to build with MacPorts' wxWidgets (v3.0.4) and it looks like dark mode definitely doesn't behave well with freedv-gui. However, the app itself looks okay in the default "light" mode. Perhaps we can just enforce 3.1 with CMake when building on Mac unless one is building on an old enough version of the OS?

👍 Good idea @tmiw - I'm happy for you to make the call on OSX issues like this. Feel free to add the code to enforce wxWidgets 3.1 for OSX in this PR.

@ra1nb0w I'm new to tags - will work out how to add it over the weekend. I'm unusually busy with $dayjob atm so sorry I'm a bit slow to respond.

A lot of Linux people will continue to build with distro-supplied wxWidgets (e.g. 3.0) so we just need to make sure that isn't broken. For Windows we can probably just settle on any version (e.g. fix it like for OSX), as that is built by very few people and tends to get distributed as an executable.

How about we bump the freedv-gui version to 1.4.3 in this PR?

@hobbes1069 do you have any thoughts on this PR?

@tmiw
Copy link
Collaborator Author

tmiw commented Aug 7, 2020

Thanks @ra1nb0w for the tip regarding NSRequiresAquaSystemAppearance. Based on that, I was able to update CMakeLists.txt to disable dark mode support (by generating the appropriate info.plist) if building with wxWidgets 3.0 and confirmed that the app works properly with both 3.0 and 3.1 on Mac. I also bumped up the version number to 1.4.3.

@hobbes1069
Copy link
Collaborator

I've been following the conversation. All seems reasonable. Fedora is still on version 3.0.4, Rawhide (rolling devel branch) is on 3.0.5.1, so it doesn't look like we're jumping on the 3.1 train just yet.

@ra1nb0w
Copy link
Contributor

ra1nb0w commented Aug 7, 2020

I also bumped up the version number to 1.4.3.

conflict. rebase because upstream has version 1.4.2. In fact you are using and tree.

@drowe67 git tag is easy git tag 1.4.2 bb9558cc496b7b253fb54bb28486b0bcd7516ae5 then you can create the release from github. You can also do everything from the github web interface.

Just a side note: I am not sure that creating a new tag from the latest commit is a good idea since it requires latest codec2 freedv_get_hash() and lpcnet lpcnet_get_hash()

@tmiw
Copy link
Collaborator Author

tmiw commented Aug 7, 2020

@hobbes1069, Debian Bullseye is only on 3.0.5.1 too. Not sure there are any Linux distros with 3.1.

@ra1nb0w, conflict merged.

@drowe67
Copy link
Owner

drowe67 commented Aug 8, 2020

@tmiw I managed to build this PR against wxWidgets 3.0 and 3.1 for Linux, and the dialogs modified look and (with a basic test) work OK. Can decode some audio files OK.

I ran into problems building for Windows, due a local Docker configuration issue. I don't have time to look into that right now. So I'll merge this PR and if any Windows problems emerge we'll deal with them later.

@drowe67
Copy link
Owner

drowe67 commented Aug 8, 2020

@hobbes1069 - we spoke recently on the mailing list about a release for 1.4.2 but I understand you didn't think we needed one? However @ra1nb0w has requested a tag (at least). Can you pls work with @ra1nb0w to work out a way fwd?

@ra1nb0w - @hobbes1069 has been managing releases for freedv-gui.

@drowe67 drowe67 merged commit 4d14643 into drowe67:master Aug 8, 2020
@ra1nb0w
Copy link
Contributor

ra1nb0w commented Aug 9, 2020

note: I am not sure that creating a new tag from the latest commit is a good idea since it requires latest codec2 freedv_get_hash() and lpcnet lpcnet_get_hash(). Otherwise you should also tag a new version for codec2 and lpcnet.

@hobbes1069
Copy link
Collaborator

Tags can be arbitrary (non-version related) if we just need a known state of code to work with but I agree with @ra1nb0w that if we want to tag a version we'll need to create tags/releases for codec2 and lpcnetfreedv for a known compatible combination.

@ra1nb0w
Copy link
Contributor

ra1nb0w commented Sep 2, 2020

I mean tag then github version.

@hobbes1069
Copy link
Collaborator

Sure, just need a commit hash that gets you what you need and agree on a tag name :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants