Refactor separate per profile settings from general ones#1388
Conversation
I anticipate a need to be able to show the options/preferences dialog on start up when there is NO profile loaded. This commit aims to make it possible to do so. It moves all the options that are not tied to a specific Host instance to the first tab - and moves any that are to elsewhere. It then disables the other tabs so they cannot be accessed. Internally it also refactors the diaProfilePreferences class so that ALL methods check for and are safe to use even if dlgProfilePreferences::mpHost is a nullptr. To allow debugging controls to still be placed onto the dialog the groupBox_Debug has been duplicated so that there is a groupBox_debugApplication on the first tab and a groupBox_debugProfile on the last - at present they are both hidden. Accordingly the mudlet::show_options_dialog() slot no longer bails out if there is not an "active" profile. The indentation in a couple of places in dlgProfilePreferences.cpp are left wrong so that the changes are more clearly visible - this will be updated in the next commit. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
There is no code change here this commit merely fixes the indentation left wrong before to show the functional changes better. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
Hey, sorry I haven't been able to review this. I'll get to it shortly. |
|
I want to keep this stuff simple, not be more complicated and then also have more tools to debug the fact that it's more complicated and can break :( |
|
The example I gave with the translation system was an example but the underlying principle is still there - it is a non-optimum arrangement that non-profile specific settings cannot be accessed while there is not a profile loaded (at the start of a run of the Mudlet application) if there is any setting that the user needs to change before they start their session they cannot get to the settings dialogue to make such a change - and if they did with the current code it would seg. fault very quickly because the code expects there to be a valid profile currently active. This PR is designed to make it safe and workable to access the settings even if there is not a profile active. One gotcha that I think this could help to prevent if it actually exists is that I understand that on the MacOS platform the As for the delay on the translation thing I have had hit a slight issue in how to handle working out what the user wants to use from the system settings they have set - although |
|
The problem of not being able to open settings before you open a profile is a problem, agreed. I'm not a fan of the UI reorganisation that happened though because it changes the settings tabs from being logical to being illogical - stuff was organised by a theme that helped the person find what setting to change, and now it's not. The proposed UI organises things by the way they are saved on disk and that is not intuitive. I understand just disabling other tabs that are profile specific is pretty easy but while it's easy for us to code this it looks pretty ugly for the user. The proper but more difficult solution would be to actually disable settings that you can't edit without a profile open. |
|
I don't want to to just say I don't like it without offering a solution -
how about a whitelist of widgets responsible for settings that are not
stored in the profile, we can then use Qts runtime introspection features
to walk the dialogue tree and disable any widgets not in the list.
…On Tue, 14 Nov 2017, 7:27 am Stephen Lyons, ***@***.***> wrote:
The example I gave with the translation system was an example but the
underlying principle is still there - it is a non-optimum arrangement that
non-profile specific settings *cannot be accessed while there is not a
profile loaded* (at the start of a run of the Mudlet application) if
there is any setting that the user needs to change before they start their
session they cannot get to the settings dialogue to make such a change -
and if they did with the current code it would seg. fault very quickly
because the code expects there to be a valid profile currently active. This
PR is designed to make it safe and workable to access the settings even if
there is not a profile active.
One gotcha that I think this could help to prevent if it actually exists
is that I understand that on the MacOS platform the QAction that
activates the settings should be given the QAction::PreferencesRole so
that it gets placed in a specific place on the *application* menu on that
platform (there are others for Help, About, AboutQt and Quit) however I do
not know whether it can be disabled on there when there is not a profile
loaded so it seems sensible to protect the code like this.
As for the delay on the translation thing I have had hit a slight issue in
how to handle working out what the user wants to use from the system
settings they have set - although bool QTranslator::load(const QLocale
&locale, const QString &filename, const QString &prefix = QString(), const
QString &directory = QString(), const QString &suffix = QString()) looks
just right to load in the best translations according to what the *user*
has said they want there is no way to find out currently what the Qt code
actually decided to use taking their selection into account. I am trying to
get my head around what all the issues that I need to account for are and
how to code a GUI to work them and deal with some Mudlet unrelated but
time-swallowing minor RL issues...
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#1388 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGxjIiCeYHrPdAg9Yh49zwq8_bh_lz0ks5s2TLBgaJpZM4QSEXG>
.
|
That might be possible - but it is more complicated because some controls are not enabled by default anyway - and the same disabling process ought to also disable the associated Doing it on a tab basis seemed the least disruptive way to do it but I agree it is not quite so nice for the UI for the controls that had to be moved around - on the other hand I do think a review of the placement of all the controls might suggest some could be constructively repositioned... Though many controls are included inside |
Go back to original form so we can approach issue a different way. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
To make it easier to enabled/disable groups of items. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
… tabs Also remove unused (obsolete) items from profile preferences dialogue/form: * (QPushButton *) pushButtonBorderColor * (QPushButton *) pushButtonBorderImage Remove incorrectly placed layout items (shows with a red outline in Qt Designer) on profile preferences dialogue/form. Correct a grammatical error of an extra "do" in the colour setting tab for the main display. Reorder as necessary QGridLayout components in profile preferences dialogue/form to put them in logical order (left-to-right, top-to-bottom). Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
@vadi2 Try it now! - It disables groupBoxes where they contain all items to disabled and the elements in the remainder that also need to be disabled if there is not a valid normal |
|
That looks perfect! One last thing, we gained a problem we didn't have before: you can open settings, connect to a profile, and the settings a little confusingly don't get enabled. Do you think we can add a special case to re-load the settings dialog once the first profile is opened? It'll be OK to do since all of the profile widgets are a superset of the no-profile settings window. |
If I have got that straight it is not as simple as you think. Do you mean opening the Preferences Dialogue before a profile is selected in the Connection Preferences Dialogue then opening one from the latter so that the former has to deal with a change of Technically the preference dialogue applies to the currently active host and initially, obviously, in this case there was not one. How does/can that usage case be separated from the multi-playing case when the user opens the preferences on one profile and then selects a different one - should the preferences then also switch from the first to the second profile - I think that is not a wise thing. tl;dr; it might not be elegant but it is safe now and I am not sure that we should try to address this corner case which can be overcome by closing and reopening the dialogue. |
|
Well, we addressed the corner case that you can't open preferences without a profile open, but we added the corner case that you can't use the preferences window at all then when you actually connect until you close and re-open. Trust me - someone will run into this and think it's a bug and that Mudlet is broken, and in that way we fixed one problem but added another. How about saving all values and closing the dialog automatically when you open the first profile? |
|
So we need to detect when we get a close event from connection preferences dialog (which may be the thing that creates a new I'll be honest - I am not sure I can get a handle on the programme logic / algorithm to achieve the desired effect without introducing unwanted side effects (or producing something that is a pile of 🍝 )... 😟 Let me see how it looks after I have give it some thought. |
These will be needed to allow the Profile Preferences Dialogue to handle the addition of a (first real) Host instance and (potentially, should other developments happen) the removal of a (last real) Host instance. Also fixed a corner case where a request to HostManager to delete a Host when the specified named Host does not actually exist - it was still returning a true even when that is not appropriate. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
Actually I think I can put together something to handle this after all - by getting the Incidentally, before I started on this PR I suspect that if there were more that one profile loaded and the Profile Preferences was opened on one that was then closed I suspect that changing the font size or selecting the search engine will cause a fatal crash as those didn't have a check for a null |
…logue
Also:
* Renamed:
dlgProfilePreferences::slot_search_engine_edited(const QString &text)
==> dlgProfilePreferences::slot_setSearchEngine(const QString &text)
* And removed unneeded use of:
dlgProfilePreferences::setSearchEngine(const QString &text)
as a separate method for it to wrap...
* Moved:
* (void) dlgProfilePreferences::slot_script_selected(int)
* (void) dlgProfilePreferences::slot_editor_tab_selected(int)
* (void) dlgProfilePreferences::slot_theme_selected(int)
from "private" to "private slots" area in header file as they ARE being
used as slots (with QObject::connection method) but were not in the
"correct" area...
* In the above "private" area of header file also grouped all the methods
together before the members whilst retaining the order of the latter -
generally this makes it easier to ensure that constructor initialisation
lists are maintained in order!
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…omGeneralOnes Resolved conflicts in: * src/dlgProfilePreferences.cpp Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
@vadi2 OK give this a spin... |
| theme_download_label->setText(tr("Updating themes from colorsublime.com...")); | ||
| } | ||
|
|
||
| void dlgProfilePreferences::slot_handleHostAddition(Host* pHost, const quint8 count) |
There was a problem hiding this comment.
Mind adding a comment here explaining what the function's purpose is? "handle host addition"... to do what?
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
Will "squash and merge" with a further 👍 from you @vadi2 . |
|
Looks good 👍 |
Brief overview of PR changes/additions
This modifies the profile preferences dialogue so that all the non-profile specific options are on the first tab and all the ones on that tab that are profile specific are moved elsewhere on the dialogue. It then disables all but the first tab if the constructor is called with a
nullptrvalue for theHost *argument. This is so that it is possible to access the dialogue even when there is NOT a profile loaded. ThedlgProfilePreferencesclass methods are also made fully safe should(Host *) dlgProfilePreferences::mpHostbe anullptr...Motivation for adding to Mudlet
I am doing this in anticipation of it being necessary to be able to access the settings on start-up when perhaps there are issues locating the translations files or if there are problems in determining which one to use - it also seemed a sensible thing to do and simpler than having completely separate "General" and "Profile" options dialogues should other "settings" be needed/adjusted before a profile is loaded - which the past code prevented! 😮
Other info (issues closed, discussion etc)
I have removed the
preferences/settingsmain toolbar button from the set of those modified by(void) mudlet::disableToolbarButtons()and(void) mudlet::enableToolbarButtons()- which allows the effects to be seen and tested by hitting theCanceloption on the auto activated Connection Profiles dialogue.Some controls have been rearranged to make better use of the space although a review of what control is where may benefit from further consideration...
The second commit may be ignored when reviewing the code changes as I deliberately left some changed code mis-indented after the first commit so it is easier to see the code modifications. The second commit merely corrects the code formatting without introducing any functional changes. As usual though, the significant changes to the
src/ui/profile_preferences.uifile are almost impossible to traces through the source file changes. I'll see if I can drop in a set of before and after screen shots for them...I will need to merge this with #1334 as there is some overlapping areas (in the
mudletclass).