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

Adding global messagebox and commandline parameter parsing classes. #2538

Open
pgScorpio opened this issue Mar 22, 2022 · 8 comments
Open

Adding global messagebox and commandline parameter parsing classes. #2538

pgScorpio opened this issue Mar 22, 2022 · 8 comments
Labels
refactoring Non-behavioural changes, Code cleanup

Comments

@pgScorpio
Copy link
Contributor

What is the current behaviour and why should it be changed?

Currently messageboxes for errors, warnings and info are created all over the code. Disadvantage is that they don't have a parent dialog and are inconsistent in message title.

Also commandline parameters are now parsed by several functions in main and passed via-via through several constructor parameters. A class accessible anywhere in the code would be more efficient and more versatile.

Describe possible approaches

All this is in preparation of the "sound-redesign"

A static messagebox class will provide messageboxes that can be opened anywhere in the code, but are always child of CClientdlg and will have a consistant title (including any clientname).

The commandline parsing functions in main will be moved to a (static) class that is accessible anywhere in the code so commandline parameters can be retrieved anywhere in the code, no need to pass then via-via through constructors.
Also we will be able to use "special" commandline parameters (normally rejected) for debugging and testing purposes.

Has this feature been discussed and generally agreed?

The commandline parsing class was already suggested in the comments in the code.

@pgScorpio pgScorpio added the feature request Feature request label Mar 22, 2022
@ann0see ann0see added this to Triage in Tracking (old) via automation Mar 22, 2022
@ann0see ann0see added the refactoring Non-behavioural changes, Code cleanup label Mar 22, 2022
@ann0see
Copy link
Member

ann0see commented Mar 22, 2022

Great! Thanks for opening an issue. I think it is possible to have a PR with only the class and some mi or code changes?

@pgScorpio
Copy link
Contributor Author

@ann0see

I think it is possible to have a PR with only the class and some mi or code changes?

As a matter of fact this can be implemented with only changes in global.h and main.cpp.
It does not have to be actually used anywhere else in the code yet...
Though the message boxes can already be used on several places, that could be another PR. Even someone else could pick up changing all message boxes to the new implementation.)

The no longer needed (nor wanted) commandine parameter passing mainly affects CSound so I will make these changes automatically in following PR's.

@ann0see
Copy link
Member

ann0see commented Mar 22, 2022

I think everything in the scope of this issue can be put into one PR.
Other people should comment, but I‘d be fine with not splitting the error messages out.
I‘d not be OK with big visible UI changes or sound related changes in one big PR if not absolutely necessary.

@pljones
Copy link
Collaborator

pljones commented Mar 23, 2022

Also commandline parameters are now parsed by several functions in main and passed via-via through several constructor parameters. A class accessible anywhere in the code would be more efficient and more versatile.

I'm working on that.

@pgScorpio
Copy link
Contributor Author

pgScorpio commented Mar 23, 2022

@pljones

I'm working on that.

I already have it in my own jamulus repo and I would like to push it but, again, I don't know how to open a PR for this issue and push it to jamulus master.

@ann0see (Hate to say it, but I'm getting really tired of all those github related issues.)

EDIT: I figured it out again I think. Is this the right way ? (Still says PR not found ?)

@ann0see
Copy link
Member

ann0see commented Mar 23, 2022

@pgScorpio let‘s have a Video call this evening. When would you be available?

@pgScorpio
Copy link
Contributor Author

@ann0see
Wednesday evenings I have my choir rehearsals, any other evening I'm available most of the time though.

@ann0see
Copy link
Member

ann0see commented Mar 23, 2022

Ah ok. Would tomorrow evening be ok? Probably we should limit it to ~ 1h. I‘d prefer to not start before 20:00

pgScorpio added a commit to pgScorpio/jamulus that referenced this issue Apr 2, 2022
pgScorpio added a commit to pgScorpio/jamulus that referenced this issue Apr 6, 2022
pgScorpio added a commit to pgScorpio/jamulus that referenced this issue Apr 6, 2022
pgScorpio added a commit to pgScorpio/jamulus that referenced this issue Apr 7, 2022
@pljones pljones removed this from Triage in Tracking (old) Jul 28, 2023
@pljones pljones removed the feature request Feature request label Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Non-behavioural changes, Code cleanup
Projects
Status: Triage
Development

Successfully merging a pull request may close this issue.

3 participants