Scrolling text in Input widget

[prl]

Perhaps, but it really should be fixed in the code, as

Code: Select all

     if (markedpos >= (int)glyphs.size())

noting that

Code: Select all

     if ((size_type)markedpos >= glyphs.size())

won't test correctly for allowed values of marked_pos.

At least in this case markedpos is already known >= 0 at this point.

Yes, but the compiler may well not know that.

I really don't think it's a good idea to "remove" legitimate warnings by having the compiler ignore them.

I look forward to your patch to fix all the existing signed/unsigned warnings.

Believe me, I've been tempted to. The reason why I haven't, at least for C++ compilation of code that's not machine-generated, is to avoid unnecessary potential upstream merge problems. There aren't actually a huge number of them in the non-machine generated part of the code.

What would be a big job, though, would be to get rid of all the gettext preprocessor "'msgid' format string with unnamed arguments cannot be properly localised" warnings in the Python code.

But IMO the existence of unnecessary warnings in the current code in no way justifies adding them in new code.

[IanSav]

Hi,

Adoxa's changes appear quirky at first but I am quickly getting used to the cursor pausing in the middle of the control and the text scrolling until the end boundary is visible.

How hard would it be to have the pause location default to the centre but to allow the user to specify the pause point? Perhaps a positive number to set the location relative to the start of the window and a negative number to set the location relative to the end of the window and zero to indicate the centre of the string. It would be great if this pause position could be specified in the skin so that the window size, pause location and text alignment can all be specified in the same place.

I think users could grow to like the feature.

Regards,
Ian.

[IanSav]

Hi Prl,

If you want help to get warnings addressed in the upstream images then I would be willing to help. I would just ask that we get some of the higher priority items off our desks first.

Regards,
Ian.

[prl]

Adoxa's changes appear quirky at first but I am quickly getting used to the cursor pausing in the middle of the control and the text scrolling until the end boundary is visible.

One problem is that with these changes, scrolling in both Input and ConfigText widgets works in two different ways, depending on whether visible_text is set to 0 or not. If visible_text is 0, scrolling is adoxa's "keep the cursor in the centre and scroll the text", and if it is non-zero, it's "only scroll the text if the cursor would become non-visible".

Compare the scrolling in MENU>Setup>TV>EPG settings>EPG cache filename with the scrolling in MENU>Sources / Files, BLUE Rename.

How hard would it be to have the pause location default to the centre but to allow the user to specify the pause point? Perhaps a positive number to set the location relative to the start of the window and a negative number to set the location relative to the end of the window and zero to indicate the centre of the string. It would be great if this pause position could be specified in the skin so that the window size, pause location and text alignment can all be specified in the same place.

I'd rather see the scrolling work in a similar way to how scrolling works currently works when visible_width is non-zero (e.g. as in MENU>Sources / Files, BLUE Rename), and have it work that way from the first beta on. That would require a new data member in the classes doing the scrolling to hold the current display offset, analogous to the offset in the Input widget.

[prl]

I'd also like to suggest that the part of the changes to elistboxcontent.cpp that stops the value in ConfigText from overwriting the label be done in a separate commit. It's a change that is worthwhile doing independently of any change in text scrolling.

[prl]

I've seen an odd interaction between adoxa's scrolling changes and the easy-skin-aus-hd skin in MENU>Setup>TV>EPG settings>EPG cache filename.

The highlighted text value of the EPG cache filename looks a bit weird:

The immediate cause seems to be that in the SetupConfig screen skin in easy-skin-aus-hd/skin_setup.xml, the config widget skin contains borderWidth="1". The equivalent skin widgets in Full-Metal-Wizard and OverlayHD don't have a borderWidth attribute, and display normally.

If adoxa's patches aren't installed, EPG cache filename is highlighted normally. If the borderWidth="1" attribute is removed from the easy-skin-aus-hd skin, EPG cache filename is highlighted normally both in the distributed firmware and with adoxa's scroll patches.

There doesn't seem to be any good reason for the borderWidth attribute in this context, but there probably shouldn't be a change in behaviour for it between the distributed and the patched code. One of the strange things about it is that the highlighted text colour also changes from blue to black when it becomes "fuzzy".

[prl]

[T]he issue in the SMS-input information panel (NumericalTextInputHelpDialog) that IanSav raised in the comments in the issue tracker entry for Bug #317.

That issue is now handled completely cleanly, including correct highlight height, by adoxa's new changes to text input scrolling.

[adoxa]

How hard would it be to have the pause location default to the centre but to allow the user to specify the pause point? Perhaps a positive number to set the location relative to the start of the window and a negative number to set the location relative to the end of the window and zero to indicate the centre of the string.

I think it would be better to have a number that is used relative to each edge, depending on direction. E.g. if it was 25%, then the cursor would remain 25% from the left when moving towards the start and 25% from the right when moving towards the end (ignoring bidi...). That option would solve both methods - my preference would be 50% and prl's would be 0%. It doesn't help with the hybrid option - perhaps negative could be used for that? E.g. -25% would scroll both cursor and text. Or just have another setting. Or don't worry about it. Yeah, might not worry about it. Your "pause location" would be simpler to implement, but I think it would be weird if it was set near the left and you were moving to the end (I had just that scenario during testing).

It would be great if this pause position could be specified in the skin so that the window size, pause location and text alignment can all be specified in the same place.

It should also be a global option, somewhere in GUI settings? Any ideas for a name? As a skin parameter pauseLocation works fine for me, or maybe cursorPause. Straight number would be pixels ("100" means stop 100 pixels from the edge), or with a percent symbol to set relative to the size ("50%" means stop in the middle). Or maybe the skin only needs pixels? The option would only be percentage. As for option name, how about "Cursor scroll position"? Description: "Once the cursor is at this position it will remain there as long as possible, with the text scrolling around it."

One problem is that with these changes, scrolling in both Input and ConfigText widgets works in two different ways, depending on whether visible_text is set to 0 or not. If visible_text is 0, scrolling is adoxa's "keep the cursor in the centre and scroll the text", and if it is non-zero, it's "only scroll the text if the cursor would become non-visible".

I am aware of the difference in scroll behaviour (even more so, as I have another patch to scroll by five characters at a time, so moving through a timer description was so slow). That's a reason to remove most uses of visible_width.

I'd also like to suggest that the part of the changes to elistboxcontent.cpp that stops the value in ConfigText from overwriting the label be done in a separate commit. It's a change that is worthwhile doing independently of any change in text scrolling.

Fair enough, but I'd rather leave it as part of this, since it's a bit simpler to keep it as a single change.

I've seen an odd interaction between adoxa's scrolling changes and the easy-skin-aus-hd skin in MENU>Setup>TV>EPG settings>EPG cache filename.

I'll look into it...

[prl]

I'd also like to suggest that the part of the changes to elistboxcontent.cpp that stops the value in ConfigText from overwriting the label be done in a separate commit. It's a change that is worthwhile doing independently of any change in text scrolling.

Fair enough, but I'd rather leave it as part of this, since it's a bit simpler to keep it as a single change.

I'm not sure why it is makes it significantly more complicated to split the changes into two functionally unrelated commits.

[adoxa]

I've seen an odd interaction between adoxa's scrolling changes and the easy-skin-aus-hd skin in MENU>Setup>TV>EPG settings>EPG cache filename.

I've explicitly disabled the border when the marked position is set (update pushed, but I won't release a new patch).

Ian, how keen are you on having the cursor scroll position/pause location as part of the skin? I don't really see it as a skinnable option (it's more a user preference) and it looks like a bit of work to add.

[prl]

I've seen an odd interaction between adoxa's scrolling changes and the easy-skin-aus-hd skin in MENU>Setup>TV>EPG settings>EPG cache filename.

I've explicitly disabled the border when the marked position is set (update pushed, but I won't release a new patch).

Thanks.

I'll get rid of the borderWidth attribute in the skin, too. It really shouldn't be there.

[IanSav]

Hi Adoxa,

Ian, how keen are you on having the cursor scroll position/pause location as part of the skin? I don't really see it as a skinnable option (it's more a user preference) and it looks like a bit of work to add.

I wouldn't rate it as a must have but I think the ability for users / skinners to be able to set the pause point could be welcomed. As I noted earlier, it first appeared quirky to me. If the pause point was two or three characters from the right margin I don't think I would have made the "quirky" comment. I can foresee that the location I like may not suit others hence the configurability suggestion.

If the location were skinnable, either in the control itself or via a parameter then it becomes a choice that users or skinners can tweak without cutting or changing any code.

Regards,
Ian.

[adoxa]

I added the "Cursor scroll position" option to the bottom of "GUI Settings":

Code: Select all

<item level="0" text="Cursor scroll position" description="The point at which the text will scroll to the cursor (percentage of the width).">config.usage.cursorscroll</item>

set it in InitUsageConfig:

Code: Select all

config.usage.cursorscroll = ConfigSelectionNumber(min=0, max=50, stepwidth=5, default=0, wraparound=True)

and accessed it in eTextPara::renderString:

Code: Select all

int cursorscroll = eConfigManager::getConfigIntValue("config.usage.cursorscroll");

Changing the option works fine, but accessing it generates SIGSEGV (in Python, apparently). Am I missing something?

[prl]

I can't see anything obvious.

Does the first call of eConfigManager::getConfigIntValue("config.usage.cursorscroll") happen after config.usage.cursorscroll is defined?

Personally, I don't really see the point of this change and I'd rather that the effort was used to implement "don't move the scroll window while the cursor remains inside it" scrolling, as done already when visible_length is non-zero.

[adoxa]

Does the first call of eConfigManager::getConfigIntValue("config.usage.cursorscroll") happen after config.usage.cursorscroll is defined?

I would expect so, since I can change the option. Looks like I'll have to debug it the hard way...

Personally, I don't really see the point of this change and I'd rather that the effort was used to implement "don't move the scroll window while the cursor remains inside it" scrolling, as done already when visible_length is non-zero.

This change implements that, but as an option so I can have my middle scroll, and Ian can have it almost at the edge (if that's what he wanted).

[prl]

Personally, I don't really see the point of this change and I'd rather that the effort was used to implement "don't move the scroll window while the cursor remains inside it" scrolling, as done already when visible_length is non-zero.

This change implements that, but as an option so I can have my middle scroll, and Ian can have it almost at the edge (if that's what he wanted).

OK, thanks. I've clearly misunderstood the intention of the change.

[IanSav]

Hi Adoxa,

I am not sure that making this a config option is a good choice. As I mentioned previously, I think this would be much better as a skin parameter. Keeping all the configuration in one place is to be preferred. In this case the settings should all be in the skin.

Regards,
Ian.

[adoxa]

So if I want all my input boxes to scroll in the middle you want me to change every input box in the skin? Bugger that.

[prl]

So if I want all my input boxes to scroll in the middle you want me to change every input box in the skin? Bugger that.

It seems more natural to me that this is a user option rather than in the skin.

[IanSav]

Hi Adoxa,

So if I want all my input boxes to scroll in the middle you want me to change every input box in the skin? Bugger that.

?!?!?! No, that is not what I said.

I suggested: <parameter name="CursorScroll" value="50" />

If you really want this as a Setup setting option then it will need a much better description.

Regards,
Ian.

[adoxa]

Ah, that makes more sense. I still don't see it as a skin option, though, but a user preference.

Yes, the description needs improving.

[adoxa]

Updated the T3 patch. I've changed the description, but it probably still needs improving.

That was a lot of work to make it scroll somewhere other than the centre. It seems accessing Python within redraw doesn't work, so I read the option in gPainter::renderText. I'm not really happy with how I've done it, but I'm not really happy with the alternatives, either.

[adoxa]

There's a bug causing long lines (without a mark) to be displayed incorrectly. I've pushed the fix, but not updated the patch.

[prl]

I'm going to be off-net and away from my Beyonwiz gear from tomorrow until Fri 28 Sep.

[IanSav]

Hi Prl,

I look forward to your return.

Regards,
Ian.

[prl]

Thanks

[Gully]

I'm going to be off-net and away from my Beyonwiz gear from tomorrow until Fri 28 Sep.

Have a great break.

[prl]

Thanks again