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.

Sign up for GitHub

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

Jump to bottom

#2850 Remove --cleanuplegacyfadersettings command line option #3116

Merged
merged 1 commit into from
Oct 2, 2023
Merged

#2850 Remove --cleanuplegacyfadersettings command line option #3116

merged 1 commit into from
Oct 2, 2023

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Jul 29, 2023 •
edited by ann0see
Loading

Short description of changes

Removes undocumented --cleanuplegacyfadersettings command line option.

CHANGELOG: SKIP

Context: Fixes an issue?

Fixes: #2850

Does this change need documentation? What needs to be documented and how?

No.

Status of this Pull Request

Tested

  1. Option no longer available:
$ ./Jamulus --cleanuplegacyfadersettings
./Jamulus: Unknown option '--cleanuplegacyfadersettings' -- use '--help' for help
  1. ./Jamulus after local build:

Before run:

 <storedfadertag0_base64>Tm8gTmFtZQ==</storedfadertag0_base64>
 <storedfadertag1_base64>QXllIE1pbiBIYW4=</storedfadertag1_base64>
 <storedfadertag2_base64>RGFuaWVsIEh0b28=</storedfadertag2_base64>
 <storedfadertag3_base64>SGFu</storedfadertag3_base64>
 <storedfadertag4_base64></storedfadertag4_base64>
 <storedfadertag5_base64></storedfadertag5_base64>
 <storedfadertag6_base64></storedfadertag6_base64>

Connected to a server, exited.

After run:

 <storedfadertag0_base64>Tm8gTmFtZQ==</storedfadertag0_base64>
 <storedfadertag1_base64>QXllIE1pbiBIYW4=</storedfadertag1_base64>
 <storedfadertag2_base64>RGFuaWVsIEh0b28=</storedfadertag2_base64>
 <storedfadertag3_base64>SGFu</storedfadertag3_base64>
 <storedfadertag4_base64></storedfadertag4_base64>
 <storedfadertag5_base64></storedfadertag5_base64>
 <storedfadertag6_base64></storedfadertag6_base64>

What is missing until this pull request can be merged?

Code review - deleting previously added code.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@pljones pljones added this to the Release 3.11.0 milestone Jul 29, 2023
@pljones pljones self-assigned this Jul 29, 2023
@pljones pljones linked an issue Jul 29, 2023 that may be closed by this pull request
My Profile tab sometimes disappears #3114
Closed
@pljones pljones removed a link to an issue Jul 29, 2023
My Profile tab sometimes disappears #3114
Closed
@pljones pljones linked an issue Jul 29, 2023 that may be closed by this pull request
Remove --cleanuplegacyfadersettings command line option #2850
Closed
@ann0see
Copy link
Member

ann0see commented Jul 29, 2023

This should be documented, so no SKIP?

@pljones
Copy link
Collaborator Author

pljones commented Jul 29, 2023 •
edited
Loading

I'd rather it remained undocumented. In #2839, the change log was

Client: Bugfix: Remove channel number from saved fader tag when --ctrlmidich is in use. In this release --cleanuplegacyfadersettings can be used on any saved fader settings that have been corrupted to attempt recovery.

-- the --cleanuplegacyfadersettings was documented there as "in this release" (3.9.1). No one should be expecting to work in 3.10.0, and certainly not 3.11.0.

@ann0see
Copy link
Member

ann0see commented Jul 29, 2023

No one should be expecting to work in 3.10.0, and certainly not 3.11.0.

This implies we should also remove it from the website.

@pljones
Copy link
Collaborator Author

pljones commented Jul 29, 2023

This implies we should also remove it from the website.

It was meant to be an undocumented option. There shouldn't be any mention of it other than discussions / issues.

@pljones pljones force-pushed the patch/2850-remove-cleanuplegacyfadersettings branch 2 times, most recently from a7b3968 to 6f680ae Compare August 1, 2023 17:34
@pljones
Copy link
Collaborator Author

pljones commented Aug 1, 2023

I'm seeing lots of

C:\Qt\6.5.2\msvc2019_64\include\QtGui/qtextoption.h(118): fatal error C1060: compiler is out of heap space

@ann0see
Copy link
Member

ann0see commented Aug 1, 2023 •
edited
Loading

Yes. I think I also saw that once.

@pljones pljones force-pushed the patch/2850-remove-cleanuplegacyfadersettings branch from 6f680ae to 90c811f Compare August 3, 2023 18:35
@pljones pljones added refactoring Non-behavioural changes, Code cleanup and removed refactoring Non-behavioural changes, Code cleanup labels Aug 10, 2023
@pljones pljones force-pushed the patch/2850-remove-cleanuplegacyfadersettings branch from 90c811f to 67c4a8c Compare August 15, 2023 16:16
@pljones pljones added the refactoring Non-behavioural changes, Code cleanup label Aug 19, 2023
@pljones pljones force-pushed the patch/2850-remove-cleanuplegacyfadersettings branch 4 times, most recently from 855df7a to 08867ae Compare August 28, 2023 12:31
@pljones pljones force-pushed the patch/2850-remove-cleanuplegacyfadersettings branch 2 times, most recently from e46609a to 837a829 Compare September 12, 2023 16:19
ann0see
ann0see reviewed Sep 14, 2023
View reviewed changes
src/settings.cpp
@@ -232,8 +232,6 @@ void CClientSettings::ReadSettingsFromXML ( const QDomDocument& IniXMLDocument,
int iValue;
bool bValue;

bCleanUpLegacyFaderSettings = CommandLineOptions.contains ( "--cleanuplegacyfadersettings" );
Copy link
Member

Choose a reason for hiding this comment

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

Is passing ComandLineOptions still needed?

Otherwiese we don't fully undo https://github.com/jamulussoftware/jamulus/pull/2839/files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's used in ReadSettingsFromXML in CSettings generally - it should be issuing an unused variable warning... Hm.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure? I don't find it in this function anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

src/settings.cpp:36:    ReadSettingsFromXML ( IniXMLDocument, CommandLineOptions );

@pljones pljones force-pushed the patch/2850-remove-cleanuplegacyfadersettings branch from 837a829 to 0383224 Compare September 18, 2023 18:03
@pljones pljones force-pushed the patch/2850-remove-cleanuplegacyfadersettings branch from 0383224 to 8c9f432 Compare September 27, 2023 08:23
@pljones pljones force-pushed the patch/2850-remove-cleanuplegacyfadersettings branch from 8c9f432 to 0f89d2d Compare September 27, 2023 12:45
ann0see
ann0see approved these changes Sep 30, 2023
View reviewed changes
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Ok. Then approving.

@pljones pljones merged commit d1dfe64 into jamulussoftware:main Oct 2, 2023
11 checks passed
@pljones pljones deleted the patch/2850-remove-cleanuplegacyfadersettings branch October 2, 2023 17:47
@pljones pljones mentioned this pull request Jul 28, 2024
Open
62 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ann0see ann0see ann0see approved these changes

Assignees

@pljones pljones

Labels
refactoring Non-behavioural changes, Code cleanup
Projects
Tracking
Status: Done
Milestone
Release 3.11.0
Development

Successfully merging this pull request may close these issues.

Remove --cleanuplegacyfadersettings command line option
2 participants
@pljones @ann0see