Reviewing merge request #2501: Fixes: NB#297725, Cut/Copy option is displayed unnecessarily when "com" is sugested
This is a partial fix for NB#297725. Introduce a new function MTextEdit::setControlledSelection to be able to select text without text selection controls becoming visible (selection handles and CPP widget). Whenever a user makes a selection with finger or shift+arrows, the selection controls will be visible even if selection was initially done programmatically without controls.
Updated according to feedback.
- Requested by:
Antti Pulakka - Status: Rejected
- Source repository: apulakkas-libmeegotouch:fix-browserselection
- Target repository: libmeegotouch:Harmattan_1.3_proper
- Created at: February 14th, 2012 13:12
Commits that would be merged:
- b484988
- 470d4b3
Fixes: NB#297725, Cut/Copy option is displayed unnecessarily when "com" is sugested
b484988-470d4b3Comments
Pushed new version 1
I'd like to have unit tests for that new API.
+ void setControlledSelection(int anchorPosition, int length, bool useBoundaries = false,
I think that it is unnecessary to introduce new method name here.
New overloaded version of setSelection with added showControls will be enough. Old setSelection should call the new one with showControls= MTextEdit::ShowSelectionControls.
Marcin Miklas |
2012-02-15 10:59:01 UTC
on src/corelib/widgets/mtextedit.h
+ void selectionChanged(MTextEdit::SelectionControl);
I don’t understand why you have added this signal to MTextEditSignalEmitter and not to MTextEdit directly. MTextEditSignalEmitter is used for optimizing emission of scenePositionChanges().
Marcin Miklas |
2012-02-15 11:23:52 UTC
on src/corelib/widgets/mtextedit_p.h
void setControlledSelection(int anchorPosition, int length, bool useBoundaries = false,think that it is unnecessary to introduce new method name here.
New overloaded version of setSelection with added showControls will be enough. Old setSelection should call the new one with showControls= MTextEdit::ShowSelectionControls.
Overloading would not be source compatible and will lead to compile time errors.
I don’t understand why you have added this signal to MTextEditSignalEmitter and not to MTextEdit directly. MTextEditSignalEmitter is used for optimizing emission of scenePositionChanges().
I try to avoid adding a new signal to the public API. MTextEditSignalEmitter makes it possible to hide the signal.
+ * \brief Selects text from position anchorPosition and for length characters.
could use \a before “anchorPosition” and “length” to emphasize variables
Jarno Malmari |
2012-02-15 12:38:41 UTC
on src/corelib/widgets/mtextedit.h
+ enum SelectionControl { + //! Selection controls will not be shown + HideSelectionControls, + + //! Selection controls will be shown + ShowSelectionControls + };
While having enumerations as verbs are descriptive when calling the setControlledSelection method, they might be confusing when checking them later in other parts of code. I think it is preferred to declare them as states (nouns) rather than actions (verbs).
for example:
SelectionControls {
NoSelectionControls,
LollipopsAndToobar
};
Or something.. what do you think? Would leave room to implement only Lollipops or only Toolbar ;)
EDIT: “lollipops” is used in layout guide, in code we refer to those as selection handles so maybe that would be better
Jarno Malmari |
2012-02-15 12:49:20 UTC
on src/corelib/widgets/mtextedit.h
I'd suggest names like EditingSelection and SuggestingSelection, to highlight purpose of the selection
I think that adding:
void setSelection(int anchorPosition, int length, bool useBoundaries, MTextEdit::SelectionControl showControls);
will be source compatible, it do not see any reason how it could cause compile time errors. But, I’ll check :)
Would it be possible to use text edit model to pass the selection state to view? I believe that would be the preferred way of communicating between MTextEdit and MTextEditView.
MTextEditSignalEmitter was introduced to handle setting the ItemSendsScenePositionChanges flag in one centralized place. Do we want to extend that “optimization hack” to include other logic from MTextEdit? Not to mention we are further touching MTextEditPrivate from within view :)
if someone is still reviewing feel free to put back to reviewing state!
Ok, checked that adding new method with same name but with extended parameters doesn’t break anything.
When we are talking about separation view/controller we should also add this new enum to MTextEditModel simillary to EditMode, LineMode, EchoMode.
Ok, checked that adding new method with same name but with extended parameters doesn’t break anything.
According to article: http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++ it’s not ok to add an overload to a function that hasn’t been overloaded before.
Right, in theory yes, but only if some app is taking pointer to this method, which shouldn’t be happening. We did this couple of times, I mean added overloaded method, and nothing bad happened. But If you want to be extrasafe, you can change the name of method.
There will be compile errors because seSelection has a default parameter. Consider the following. In mtextedit.h we would have:
void setSelection(int anchorPosition, int length, bool useBoundaries = false);
and
void setSelection(int anchorPosition, int length, bool useBoundaries = false, MTextEditModel::SelectionControl showControls = MTextEditModel::ShowSelectionControls);
If setSelection is called without defining value for ‘bool useBoundaries’ (e.g. setSelection(foo, bar);), the compiler would not be able to disambiguate which version to use.
That’s why I propose that added function shouldn’t have any default parameters.
Pushed new version 2
Updated with:
– New overload for MTextEdit::setSelection() instead of new function name
– Information about selection controls added to MTextEditModel
– New unit test
Now it is ok. Maybe there should be couple of unittest instead of this huge one, but it is not so important :).
+ enum SelectionControl {
I think that plural SelectionControls is better
Marcin Miklas |
2012-02-17 10:11:32 UTC
on src/corelib/widgets/mtexteditmodel.h
merged to master. i renamed the enum to plural
Changed status to merged
Changed status to merged
Not merged to target branch (1.3) so status might be misleading. I'm going to reject these foreverwaiting mrqs. We’ll cherry pick them from master if we get a chance.
Thanks.
Add a new comment:
Login or create an account to post a comment