Prevent ftl errors in Keycloak log

Follow-up of #63
This PR publishes beta releases.
It won't make it into main until this is achieved.
If you want to contribute, thank you and submit your PRs to the branch doge_ftl_errors rather than to main.

Thanks @garronej you are awesome mate!

@garronej after applying the beta version
we had many places to be changed, which we wanted to avoid. Actually it even broke some things.
 Do you think there is a chance we can prevent the ftl changes or get a version, without the need to migrate our current keycloakify based project and UIs?
I suspect the beta version you generated is based on 4.2.14 and we are on latest (4.2.13)?

Hi @marziman,
There is almost no breaking changes between v2 and v4.
You need to install the peer dependencies required:  yarn add keycloakify @emotion/react tss-react powerhooks
Install powerhooks and update tss-react to the current version. You are locked on v0.X.X.
There is also useKcMessages that is a bit different.
And finally if you have some missing pages in your KcApp.tsx that are new and you don't have, add them.
If you want up to date examples you have the two template repos and the code of the login of our platform.

Do you think there is a chance we can prevent the ftl changes or get a version, without the need to migrate our current keycloakify

You are asking for custom work that doesn't benefit the open-source community here. So ok but my rate is 100€/hour.

No actually I am not asking for custom work :-) We can do that our own.
I just did not expect that there is changes to be done, if the Keycloakify generator is modified to be relaxed on errors.
If there are changes to be done, than this is breaking. What I asked was a backport of supressing errors, which are spamming the logs and slowing the whole Keycloak down. I am not sure what is custom on that.
I did the things above but had several errors. I will give it another try.
Also I could just pick the changes and apply them myself.

You are asking for custom work that doesn't benefit the open-source community here. So ok but my rate is 100€/hour.

Every discussion here benefits the OSS community and especially Keycloakify. I am not sure how you come to these conclusions. When someone had issues with their versions of forks, I always help them. Maybe you have a differen view on OSS maintainership and must be getting much requests here (which I can not see form issues, compared to the amount of issues of other projects).
Still thank you for you answer.
BR Mehmet

Sorry giving you my rate was maybe a bit harsh but understand me.
I took time to give you a detailed answer about fonts, I updated the template repo to make it easily understandable.
I also published this beta release for you so you can benefit from @Ann2827 work before its production ready.
And now, forgive me if I miss-understood, but I thought you were asking me to publish a version of Keycloakify that would be retrocompatible with v2 that would also incorporate @Ann2827 fixes so you don’t have to update...
Don't get me wrong I love answering questions and feedback help me prioritize features. But sometimes I feel are asking a bit too much of me.

He @garronej no worries, I understand. People many times ask questions and also expect that a maintainer half works for him :-) But I am not that type. I made it work and "ported" it back to the v2. In KC 15.0.0 Errors are gone and I will observe that a bit more. In this way I will do a bit beta tester for the error logs fixing. If that works smooth for v2 than it shall make big issues for v4.
So far it looks good. I am eager to report back and will let it run for some while to see its behaviour.
BR Mehmet

Ok thank you for your understanding.
I will delete the drama if you'll allow me. First because I wish I didn't overreact. Second because I want to suppress off topic discussion.

Of course. Lets clean up here :-) I will remove some things...

@garronej Hi! In the meantime, I tried to sort through the object only by known keys. It works. But it looks like over engineering. ftl template Some types are missing in the search. But I'll probably stop developing this idea.

Hello @Ann2827 thanks for your effort.
I rewrote the whole thing yesterday. It now works fast and without logging errors, it prints out well-formatted JS and it's much better FreeMarker code overall🍾

The big breakthrough was the realization that using foo.bar?? we can test if a property is dereferencable without throwing if it isn't.
I only tested with login.ftl but there are still some properties that throw when dereferenced.
Namely url. loginUpdatePasswordUrl, url. loginUpdateProfileUrl, url.loginUsernameReminderUrl and url.loginUpdateTotpUrl.
I did what I suggested about them, I implemented a blacklist so that these properties can be skipped.  I allowed myself to do that because it's never used in the base theme but I suspect it won’t throw in all conditions, sometimes these value might be needed.
I would like to avoid logging error as much as possible but I'd rather log inconsequential error that potentially preventing some use case from being implemented. I am going to see with the Keycloak team about that.

@Ann2827 A new beta have been released if you want to try it out.

@garronej
I will definitely try the new version tomorrow.
On register.ftl template, you may encounter a problem with auth.attemptedUsername . And this is already bad. If keys: loginUpdatePasswordUrl , loginUpdateProfileUrl , loginUsernameReminderUrl  and loginUpdateTotpUrl  are not used anywhere, then key attemptedUsername is used on login-reset-password.ftl template. But if you try to get the value by key auth.attemptedUsername  on login.ftl and login-reset-password.ftl templates  (I didn't check the other templates), then you won't get an error.
On login-update-profile template, you may encounter a problem with updateProfileCtx object. And for saml-post-form.ftl template it will be an error to get the value by key url.loginAction.
Perhaps Keycloak will be able to return only those values that Freemarker templates will be able to read without errors? For example null. FreeMarker can check foo.bar?? or foo.bar!"" if the value cannot be obtained. But FreeMarker can't read extended_hash+string.

@Ann2827 Thank you for your investigation! I added special cases for the cases you mentioned.
I will create special cases like this whenever a new ftl error is reported to me.
I will merge this now a publish a new release.

4.2.14 is out.


Fixed 'specifics' numbering to reflect 'en' lesson changes

There were only two languages that needed updated (en & vi) regarding the specifics lessons. The other languages are yet to add any content to this section.

Thank you @pjhampton


Better target template with logging, error, better readme

Description

add logging use core sdk
add error use core sdk
change package.json
improve README

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):
Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] I have signed the Adobe Open Source CLA.
[x] My code follows the code style of this project.
[x] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

@shazron


Outdated region used to update cursor when 'splitkeep' is not "cursor"

Problem:    Outdated topline and botline may be used to update the cursor
position when 'splitkeep' is not "cursor".
Solution:   Update topline when cursor is set and validate botline when
adjusting cursor position for 'splitkeep'.
Are we okay with always calling update_topline() in set_cursorpos()? I don't think it's harmful but I guess it is not necessary when 'splitkeep' is "cursor". I don't think it can be avoided for when 'splitkeep' is not cursor.

Problem:    Outdated topline and botline may be used to update the cursor
position when 'splitkeep' is not "cursor".
Solution:   Update topline when cursor is set and validate botline when
adjusting cursor position for 'splitkeep'.
Are we okay with always calling update_topline() in
set_cursorpos()? I don't think it's harmful but I guess it is not
necessary when 'splitkeep' is "cursor". I don't think it can be
avoided for when 'splitkeep' is not cursor.

Generally update_topline() is called only before using w_topline.
Both because it is an "expensive" function and because there are
situations where we want to move the cursor, do something there, then
restore the cursor and expect w_topline not to change.
In win_fix_scroll() updating w_topline to scroll to be able to see the
cursor position is disabled by setting "skip_update_topline".  It's a
clear conflict between moving the cursor and expecting the window to
scroll to be able to see where it ended up, versus resizing/scrolling
and moving the cursor to make sure it is visible in the displayed lines.
In your test you do both: Move the cursor and resize the window.  For
the first w_topline should be updated to show the cursor, for the resize
the cursor may need to be moved to show the text according to
'splitkeep'.
What we could try is to check if w_topline is valid and update it when
not.  Perhaps these lines at the start of win_fix_scroll(), before
setting skip_update_topline will work:
check_cursor_moved(curwin);
if ((curwin->w_valid & VALID_TOPLINE) == 0)
update_topline();

It's hard to predict if this breaks anything, hopefully we have enough
test coverage to notice any problems this causes.
--
hundred-and-one symptoms of being an internet addict:
110.
/// Bram Moolenaar -- @.*** -- http://www.Moolenaar.net   \
///                                                                      \
\\        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\            help me help AIDS victims -- http://ICCF-Holland.org    ///

Generally update_topline() is called only before using w_topline.
Both because it is an "expensive" function and because there are
situations where we want to move the cursor, do something there, then
restore the cursor and expect w_topline not to change.

I understand. Perhaps we can set a global in set_cursorpos() to indicate that "w_topline" should be updated in win_fix_cursor()? That way it doesn't affect 'splitkeep' == "cursor" and will only affect 'splitkeep' != "cursor" when a cursor() call is also paired with a resize (before the screen is redrawn). I really don't think we can add an update_topline() to win_fix_scroll(), elaborated below. And I do think that when a user explicitly calls cursor() the cursor should actually be moved there, even when this is paired with changing window sizes and 'splitkeep' != "cursor", like in the added test.

What we could try is to check if w_topline is valid and update it when
not.  Perhaps these lines at the start of win_fix_scroll(), before
setting skip_update_topline will work:

check_cursor_moved(curwin);
if ((curwin->w_valid & VALID_TOPLINE) == 0)
update_topline();

It's hard to predict if this breaks anything, hopefully we have enough
test coverage to notice any problems this causes.

I think we cannot call update_topline() until after win_fix_scroll() has moved the cursor into the visible region. The topline might be marked invalid despite the cursor not having moved explicitly, in which case we do not want to update the topline but move the cursor instead. Adding the suggested lines to the start of win_fix_scroll() does therefor indeed result in numerous test failures for 'splitkeep', which I do think is tested quite rigorously.
If the cursor position is set explicitly however, we need to update the topline instead. If we cannot do this unconditionally, I'll try updating this to use a global variable to see if you like that better.

I think we are fine to re-use skip_win_fix_scroll and avoid "fixing" the cursor position altogether after the cursor position was explicitly set. Updated again:
Cursor is adjusted after it was explicitly set when 'splitkeep' is not "cursor"

Problem:    Cursor is adjusted after it was explicitly set when
            'splitkeep' is not "cursor".
Solution:   Avoid adjusting cursor position for a change in window
            height after it was explicitly set.

Generally update_topline() is called only before using w_topline.
Both because it is an "expensive" function and because there are
situations where we want to move the cursor, do something there, then
restore the cursor and expect w_topline not to change.

I understand. Perhaps we can set a global in set_cursorpos() to
indicate that "w_topline" should be updated in win_fix_cursor()?

There are many more ways to move the cursor.  Setting a flag every time
the cursor is moved (by the user or by a script) is not realistic.
w_valid_cursor is used to know whether the cursor moved since the last
time w_topline was updated.  The only thing we don't know is whether
w_cursor was set by the user or as a side effect of scrolling the text
and moving the cursor into the visible text.  More about that below.

That way it doesn't affect 'splitkeep' == "cursor" and will only
affect 'splitkeep' != "cursor" when a cursor() call is also paired
with a resize (before the screen is redrawn). I really don't think we
can add an update_topline() to win_fix_scroll(), elaborated below.
And I do think that when a user explicitly calls cursor() the cursor
should actually be moved there, even when this is paired with changing
window sizes and 'splitkeep' != "cursor", like in the added test.

It's hard to say what the expected behavior is when moving the cursor
and then splitting the window, especially when 'splitkeep' is not
"cursor".
In the test the cursor is moved to line 100.  Then the window sizes are
equalized while 'slitkeep' is set to "screen".  Now does the user expect
the cursor to be still in line 100, or the text not moved up/down and
the cursor moved if needed?  I would think that the last action
overrides a previous one.  Especially if some time passes or other
commands are executed in between the two actions.

What we could try is to check if w_topline is valid and update it when
not.  Perhaps these lines at the start of win_fix_scroll(), before
setting skip_update_topline will work:

check_cursor_moved(curwin);
if ((curwin->w_valid & VALID_TOPLINE) == 0)
update_topline();

It's hard to predict if this breaks anything, hopefully we have enough
test coverage to notice any problems this causes.

I think we cannot call update_topline() until after
win_fix_scroll() has moved the cursor into the visible region. The
topline might be marked invalid despite the cursor not having moved
explicitly, in which case we do not want to update the topline but move
the cursor instead. Adding the suggested lines to the start of
win_fix_scroll() does therefor indeed result in numerous test failures
for 'splitkeep', which I do think is tested quite rigorously.
If the cursor position is set explicitly however, we need to update the
topline instead. If we cannot do this unconditionally, I'll try updating
this to use a global variable to see if you like that better.

The existing flags in w_valid, especially VALID_TOPLINE, are intended
for this.  Calling check_cursor_moved() to update w_valid should have
the same effect as a global flag to indicate the cursor was moved.
We do have to make sure the flag (and w_valid_cursor) are updated when
moving the cursor for another reason (e.g. in win_fix_cursor()).  That
is the tricky part.  But not more tricky than using a global flag.
--
Amnesia is one of my favorite words, but I forgot what it means.
/// Bram Moolenaar -- @.*** -- http://www.Moolenaar.net   \
///                                                                      \
\\        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\            help me help AIDS victims -- http://ICCF-Holland.org    ///

There are many more ways to move the cursor. Setting a flag every time the cursor is moved (by the user or by a script) is not realistic. w_valid_cursor is used to know whether the cursor moved since the last time w_topline was updated. The only thing we don't know is whether w_cursor was set by the user or as a side effect of scrolling the text and moving the cursor into the visible text. More about that below.

Yeah good point I guess.

It's hard to say what the expected behavior is when moving the cursor
and then splitting the window, especially when 'splitkeep' is not
"cursor".

In the test the cursor is moved to line 100.  Then the window sizes are
equalized while 'slitkeep' is set to "screen".  Now does the user expect
the cursor to be still in line 100, or the text not moved up/down and
the cursor moved if needed?  I would think that the last action
overrides a previous one.  Especially if some time passes or other
commands are executed in between the two actions.

Note BTW that it is not a purely synthetic test, it is a simplified version of an issue a Neovim user reported for a "jump to definition" function that does something similar. I.e. move the cursor and resize some windows. The point of 'splitkeep' is to keep screenlines stable upon window resize. If the cursor position is set explicitly and paired with a window resize in a user function like in the test, there is no point in "stabilizing" that intermediate state as it was never seen by the user. win_fix_scroll() "fixing" the cursor position is then only counter productive I think, and the fact that the cursor is not in the position it was moved to is unexpected.

The existing flags in w_valid, especially VALID_TOPLINE, are intended
for this.  Calling check_cursor_moved() to update w_valid should have
the same effect as a global flag to indicate the cursor was moved.

We do have to make sure the flag (and w_valid_cursor) are updated when
moving the cursor for another reason (e.g. in win_fix_cursor()).  That
is the tricky part.  But not more tricky than using a global flag.

I can investigate if the w_valid flags can somehow be better used to achieve the desired effect. But I suspect it may look more like abusing the flags as I don't think they are designed with the current 'splitkeep' != "cursor" behavior in mind.

There are many more ways to move the cursor. Setting a flag every
time the cursor is moved (by the user or by a script) is not
realistic.  w_valid_cursor is used to know whether the cursor moved
since the last time w_topline was updated. The only thing we don't
know is whether w_cursor was set by the user or as a side effect of
scrolling the text and moving the cursor into the visible text. More
about that below.

Yeah good point I guess.

It's hard to say what the expected behavior is when moving the
cursor and then splitting the window, especially when 'splitkeep' is
not "cursor".

In the test the cursor is moved to line 100.  Then the window sizes
are equalized while 'slitkeep' is set to "screen".  Now does the user
expect the cursor to be still in line 100, or the text not moved
up/down and the cursor moved if needed?  I would think that the last
action overrides a previous one.  Especially if some time passes or
other commands are executed in between the two actions.

Note BTW that it is not a purely synthetic test, it is a simplified
version of an issue a Neovim user reported for a "jump to definition"
function that does something similar. I.e. move the cursor and resize
some windows. The point of 'splitkeep' is to keep screenlines stable
upon window resize. If the cursor position is set explicitly and paired
with a window resize in a user function like in the test, there is no
point in "stabilizing" that intermediate state as it was never seen by
the user. win_fix_scroll() "fixing" the cursor position is then only
counter productive I think, and the fact that the cursor is not in the
position it was moved to is unexpected.

The problem here is: How does Vim know that the cursor position is more
important?  Basing this on something like the two commands being short
after each other is too unpredictable.   The key point is that "resize
some windows" here is not intended to use 'splitkeep'.  Can Vim somehow
know, or does the user have to tell Vim to not use 'splitkeep'?
If 'splitkeep' would only be used for actually splitting or closing
windows, then it would work, right?  But only for this example, if a
window would be split (like when using :stag, jumping to a definition
and showing it in a new window) then the problem is there again.
I can't think of a way for Vim to know what the user's intention is.  It
looks like the only solution is that the user temporarily resets
'splitkeep'.  The luxury version would be to add a command modifier for
it.

The existing flags in w_valid, especially VALID_TOPLINE, are intended
for this.  Calling check_cursor_moved() to update w_valid should have
the same effect as a global flag to indicate the cursor was moved.

We do have to make sure the flag (and w_valid_cursor) are updated when
moving the cursor for another reason (e.g. in win_fix_cursor()).  That
is the tricky part.  But not more tricky than using a global flag.

I can investigate if the w_valid flags can somehow be better used to
achieve the desired effect. But I suspect it may look more like
abusing the flags as I don't think they are designed with the current
'splitkeep' != "cursor" behavior in mind.

Right, the use of w_valid is already quite tricky, let's not make it more
complicated.
--
hundred-and-one symptoms of being an internet addict:
120. You ask a friend, "What's that big shiny thing?" He says, "It's the sun."
/// Bram Moolenaar -- @.*** -- http://www.Moolenaar.net   \
///                                                                      \
\\        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\            help me help AIDS victims -- http://ICCF-Holland.org    ///

The problem here is: How does Vim know that the cursor position is more important? Basing this on something like the two commands being short after each other is too unpredictable. The key point is that "resize some windows" here is not intended to use 'splitkeep'. Can Vim somehow know, or does the user have to tell Vim to not use 'splitkeep'?

If 'splitkeep' would only be used for actually splitting or closing windows, then it would work, right? But only for this example, if a window would be split (like when using :stag, jumping to a definition and showing it in a new window) then the problem is there again.

I can't think of a way for Vim to know what the user's intention is. It looks like the only solution is that the user temporarily resets 'splitkeep'. The luxury version would be to add a command modifier for it.

I think basing it on whether the current cursor position was actually drawn may be a valid solution that would make the test case behave like expected. I have re-opened and updated this PR with that change. I repurposed w_last_cursorline for this, made sure that it is always updated and thus moved the w_p_cul check to the appropriate places.

Hmm, the change results in some failing 'splitkeep' tests depending on the window height. I tested locally in a larger terminal and the tests passed, in a smaller terminal it fails. I think it may be resolved by adding a :redraw somehwere.

The problem here is: How does Vim know that the cursor position is
more important? Basing this on something like the two commands being
short after each other is too unpredictable. The key point is that
"resize some windows" here is not intended to use 'splitkeep'. Can
Vim somehow know, or does the user have to tell Vim to not use
'splitkeep'?

If 'splitkeep' would only be used for actually splitting or closing
windows, then it would work, right? But only for this example, if a
window would be split (like when using :stag, jumping to a
definition and showing it in a new window) then the problem is there
again.

I can't think of a way for Vim to know what the user's intention is.
It looks like the only solution is that the user temporarily resets
'splitkeep'. The luxury version would be to add a command modifier
for it.

I think basing it on whether the current cursor position was actually
drawn may be a valid solution that would make the test case behave
like expected. I have re-opened and updated this PR with that change.
I repurposed w_last_cursorline for this, made sure that it is always
updated and thus moved the w_p_cul check to the appropriate places.

Hmm, this changes the meaning of w_last_cursorline in a way it no longer
reflects the last line where 'cursorline' was drawn.  It may happen to
work in most cases, but it looks like it now redraws the previous line
of the cursor for no good reason.  And when toggling 'cursorline' I
suspect the display may be wrong, because w_p_cul is checked at the next
redraw, not when the 'cursorline' was drawn.  Thus it may not be
redrawn when it should.  There were bugs around this in the past.
We also have w_last_cursor_lnum_rnu for a similar reason.  Adding
another field, e.g. w_last_cursor_lnum_drawn, makes sure we don't make
it more complicated and confusing.
Anyway, I'm not convinced this is the right solution.  The user doesn't
have much control over when a redraw happens, especially for avoiding
it.  Thus adding a ":redraw" after moving the cursor can change the
scrolling behavior, but there is no way to get the opposite effect.
We don't specify what commands will cause a redraw, and we may add a
redraw somewhere for any reason.  There is the 'lazyredraw' option, but
using that to change how the display scrolls is very unexpected.
So, back to why w_topline wasn't updated before scrolling?  We have the
flags to tell us w_topline needs to be updated, that is nothing new.  I
lost track of the reason why w_topline can't be updated at the start of
win_fix_scroll().  If the window was resized or scrolled before,
w_topline should already be valid.  If the cursor was moved somehow,
then updating w_topline before applying 'splitkeep' should just work,
right?
--
The difference between theory and practice, is that in theory, there
is no difference between theory and practice.
/// Bram Moolenaar -- @.*** -- http://www.Moolenaar.net   \
///                                                                      \
\\        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\            help me help AIDS victims -- http://ICCF-Holland.org    ///

Anyway, I'm not convinced this is the right solution.  The user doesn't
have much control over when a redraw happens, especially for avoiding
it.  Thus adding a ":redraw" after moving the cursor can change the
scrolling behavior, but there is no way to get the opposite effect.

Indeed, but I don't think there is any reason to get the opposite effect. Don't you agree with the statements made in the commit message/added comment in win_fix_scroll()? 'splitkeep' "topline" and "screen" are intended to make observable screen states, before and after windows changed in size, more in line with what the user expects. As such, there is no reason for 'splitkeep' to try to "stabilize" a screen state that was never seen by the user, and doing so will only result in unexpected behavior like in the added test.
I don't think this change will have any effect on singular window commands entered by the user, but it will make chained cursor movements and window commands in user scripts behave as expected. The added :redraw's in the tests only serve to simulate an observable screen state, I don't think the fact that this behavior is not reversible if a redraw already would happen matters. But I'm not totally sure either.

So, back to why w_topline wasn't updated before scrolling?  We have the
flags to tell us w_topline needs to be updated, that is nothing new.  I
lost track of the reason why w_topline can't be updated at the start of
win_fix_scroll().  If the window was resized or scrolled before,
w_topline should already be valid.  If the cursor was moved somehow,
then updating w_topline before applying 'splitkeep' should just work,
right?

Well for one, "applying 'splitkeep'" uses the old w_botline to determine the new scroll position, which update_topline() may update. Other than that I think updating w_topline should be indeed be fine in principle, but adding update_topline() to the start of win_fix_scroll() (while saving the old w_botline) still results in numerous 'splitkeep' test failures. IDK if I can be bothered debugging those tests again and I think the proposed workaround should have the same result.

Luuk van Baal wrote:

Anyway, I'm not convinced this is the right solution.  The user
doesn't have much control over when a redraw happens, especially for
avoiding it.  Thus adding a ":redraw" after moving the cursor can
change the scrolling behavior, but there is no way to get the
opposite effect.

Indeed, but I don't think there is any reason to get the opposite
effect. Don't you agree with the statements made in the commit
message/added comment in win_fix_scroll()? 'splitkeep' "topline" and
"screen" are intended to make observable screen states, before and
after windows changed in size, more in line with what the user
expects. As such, there is no reason for 'splitkeep' to try to
"stabilize" a screen state that was never seen by the user, and doing
so will only result in unexpected behavior like in the added test.

"never seen by the user" should not change what happens.  Vim optimizes
performance by skipping redraws that will soon be overwritten by
something else.  So long as there is typeahead the redraw is often
postponed.  We don't want something else to happen depending on whether
the user types fast or slow.

I don't think this change will have any effect on singular window
commands entered by the user, but it will make chained cursor
movements and window commands in user scripts behave as expected. The
added :redraw's in the tests only serve to simulate an observable
screen state, I don't think the fact that this behavior is not
reversible if a redraw already would happen matters. But I'm not
totally sure either.

So, back to why w_topline wasn't updated before scrolling?  We have
the flags to tell us w_topline needs to be updated, that is nothing
new.  I lost track of the reason why w_topline can't be updated at
the start of win_fix_scroll().  If the window was resized or
scrolled before, w_topline should already be valid.  If the cursor
was moved somehow, then updating w_topline before applying
'splitkeep' should just work, right?

Well for one, "applying 'splitkeep'" uses the old w_botline to
determine the new scroll position, which update_topline() may
update. Other than that I think updating w_topline should be indeed be
fine in principle, but adding update_topline() to the start of
win_fix_scroll() (while saving the old w_botline) still results in
numerous 'splitkeep' test failures. IDK if I can be bothered debugging
those tests again and I think the proposed workaround should have the
same result.

It appears that the reason we have such a hard time making this work
properly, is that we don't have a good definition of what should happen.
When 'splitkeep' is set to "screen" then the text of a window is
supposed to stay in the same place on the screen when the window is
moved or resized, and the cursor location is adjusted so that it is in a
valid position.  The help for 'splitkeep' explicitly says:
For the "screen" and "topline" values, the cursor position will be
changed when necessary.

But the change here is doing the opposite: it make sure the cursor stays
in the same position.  The big question is: When exactly would we use
this different behavior?
The original example was about moving the cursor and then resizing the
window, with the intention of making the text around the cursor visible.
Thus it should work like 'splitkeep' is "cursor".  But how do we know
that?  And why do something else than what the option value tells us?
Some would even call that a bug.
So, we might as well call the original example a case of wrong
expectations.  The cursor can move, because when 'splitkeep' is set to
"screen" that is what you asked for.  Expecting it to not move is
against what the option value specifies.
If the cursor must end up in a specific location, then changing the
order of commands, or temporarily setting 'splitkeep' to "cursor", can
make it work.  I do not think Vim should try to guess what the user
wants and do something that goes against the option values.
--
Statistics say that you can have a baby per month on average:
Just get nine women pregnant.
/// Bram Moolenaar -- @.*** -- http://www.Moolenaar.net   \
///                                                                      \
\\        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\            help me help AIDS victims -- http://ICCF-Holland.org    ///

Perhaps you're right, having to work around 'splitkeep' might be annoying from the perspective from plugin authors though. I still suspect there is no harm in making sure that 'splitkeep' doesn't handle screen states that weren't drawn in the first place.


0.1.17 does not compile

The 0.1.17 release is failing to build in Travis, when 0.1.16 used to work. Any idea what may be going on here?
https://travis-ci.org/serde-rs/json-benchmark/jobs/565755287

error messages
error[E0425]: cannot find value `SIMDJSON_PADDING` in this scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/stage2.rs:256:48
    |
256 |                 let mut copy = vec![0u8; len + SIMDJSON_PADDING];
    |                                                ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find value `SIMDJSON_PADDING` in this scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/stage2.rs:271:48
    |
271 |                 let mut copy = vec![0u8; len + SIMDJSON_PADDING];
    |                                                ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find value `SIMDJSON_PADDING` in this scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/stage2.rs:286:48
    |
286 |                 let mut copy = vec![0u8; len + SIMDJSON_PADDING];
    |                                                ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find value `SIMDJSON_PADDING` in this scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:142:79
    |
142 |         let needs_relocation = (buf_start + input.len()) % page_size::get() < SIMDJSON_PADDING;
    |                                                                               ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find value `SIMDJSON_PADDING` in this scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:145:62
    |
145 |             let mut data: Vec<u8> = Vec::with_capacity(len + SIMDJSON_PADDING);
    |                                                              ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find value `SIMDJSON_PADDING` in this scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:167:48
    |
167 |         let strings = Vec::with_capacity(len + SIMDJSON_PADDING);
    |                                                ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find value `SIMDJSON_PADDING` in this scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:215:40
    |
215 |         let mut copy = vec![0u8; len + SIMDJSON_PADDING];
    |                                        ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find value `SIMDJSON_PADDING` in this scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:227:18
    |
227 |         if len < SIMDJSON_PADDING {
    |                  ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find value `SIMDJSON_PADDING` in this scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:228:44
    |
228 |             let mut copy = vec![0u8; len + SIMDJSON_PADDING];
    |                                            ^^^^^^^^^^^^^^^^ not found in this scope

error[E0599]: no function or associated item named `find_structural_bits` found for type `Deserializer<'_>` in the current scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:153:31
    |
112 | pub struct Deserializer<'de> {
    | ---------------------------- function or associated item `find_structural_bits` not found for this
...
153 |                 Deserializer::find_structural_bits(&data)
    |                               ^^^^^^^^^^^^^^^^^^^^ function or associated item not found in `Deserializer<'_>`

error[E0599]: no function or associated item named `find_structural_bits` found for type `Deserializer<'_>` in the current scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:156:36
    |
112 | pub struct Deserializer<'de> {
    | ---------------------------- function or associated item `find_structural_bits` not found for this
...
156 |             unsafe { Deserializer::find_structural_bits(input) }
    |                                    ^^^^^^^^^^^^^^^^^^^^ function or associated item not found in `Deserializer<'_>`

error[E0599]: no method named `parse_str_` found for type `&'a mut Deserializer<'de>` in the current scope
  --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/serde/de.rs:21:55
   |
21 |                 visitor.visit_borrowed_str(stry!(self.parse_str_()))
   |                                                       ^^^^^^^^^^

error[E0599]: no method named `parse_str_` found for type `&'a mut Deserializer<'de>` in the current scope
  --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/serde/de.rs:78:62
   |
78 |                 return visitor.visit_borrowed_str(stry!(self.parse_str_()));
   |                                                              ^^^^^^^^^^

error[E0599]: no method named `parse_str_` found for type `&'a mut Deserializer<'de>` in the current scope
  --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/serde/de.rs:81:47
   |
81 |         visitor.visit_borrowed_str(stry!(self.parse_str_()))
   |                                               ^^^^^^^^^^

error[E0599]: no method named `parse_str_` found for type `&'a mut Deserializer<'de>` in the current scope
  --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/serde/de.rs:94:53
   |
94 |                 return visitor.visit_str(stry!(self.parse_str_()));
   |                                                     ^^^^^^^^^^

error[E0599]: no method named `parse_str_` found for type `&'a mut Deserializer<'de>` in the current scope
  --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/serde/de.rs:97:38
   |
97 |         visitor.visit_str(stry!(self.parse_str_()))
   |                                      ^^^^^^^^^^

error[E0599]: no method named `parse_str_` found for type `Deserializer<'de>` in the current scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/value/borrowed.rs:184:29
    |
184 |             b'"' => self.de.parse_str_().map(Value::from),
    |                             ^^^^^^^^^^
    |
   ::: /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:112:1
    |
112 | pub struct Deserializer<'de> {
    | ---------------------------- method `parse_str_` not found for this

error[E0599]: no method named `parse_str_` found for type `Deserializer<'de>` in the current scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/value/borrowed.rs:199:29
    |
199 |             b'"' => self.de.parse_str_().map(Value::from),
    |                             ^^^^^^^^^^
    |
   ::: /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:112:1
    |
112 | pub struct Deserializer<'de> {
    | ---------------------------- method `parse_str_` not found for this

error[E0599]: no method named `parse_str_` found for type `Deserializer<'de>` in the current scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/value/borrowed.rs:244:37
    |
244 |             let key = stry!(self.de.parse_str_());
    |                                     ^^^^^^^^^^
    |
   ::: /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:112:1
    |
112 | pub struct Deserializer<'de> {
    | ---------------------------- method `parse_str_` not found for this

error[E0599]: no method named `parse_str_` found for type `Deserializer<'de>` in the current scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/value/owned.rs:184:29
    |
184 |             b'"' => self.de.parse_str_().map(Value::from),
    |                             ^^^^^^^^^^
    |
   ::: /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:112:1
    |
112 | pub struct Deserializer<'de> {
    | ---------------------------- method `parse_str_` not found for this

error[E0599]: no method named `parse_str_` found for type `Deserializer<'de>` in the current scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/value/owned.rs:199:29
    |
199 |             b'"' => self.de.parse_str_().map(Value::from),
    |                             ^^^^^^^^^^
    |
   ::: /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:112:1
    |
112 | pub struct Deserializer<'de> {
    | ---------------------------- method `parse_str_` not found for this

error[E0599]: no method named `parse_str_` found for type `Deserializer<'de>` in the current scope
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/value/owned.rs:244:37
    |
244 |             let key = stry!(self.de.parse_str_());
    |                                     ^^^^^^^^^^
    |
   ::: /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/simd-json-0.1.17/src/lib.rs:112:1
    |
112 | pub struct Deserializer<'de> {
    | ---------------------------- method `parse_str_` not found for this

Looks like it's fixed by setting target-cpu=native in Travis. We only run cargo check there so it hadn't been needed so far. Would it make sense to have a fallback path when neither sse4.2 nor avx2 are available so that we support cargo check of crates that depend on this crate?

I suspect I do, 0.1.17 introduced SSE4.2 support and removed fallback to filler instructions for SIMD code.
Rusts default is to not use native CPU targets (or avx/sse) it seems and from looking at the .travis.yaml RUSTFLAGS is not set.
Forcing a compatible SIMD instruction-set to be enabled seems somewhat sensibel, the original change forced that, but I'm open to a discussion. It seems that 'you have to set RUSTC  flags' isn't as clear as I thought it would be.
I wonder what the best approach is.
Not forcing it leaves people with a possible bad experience since without SIMD simdjson is just json and brings no benefits over 'easier' parsers.
Forcing it means people get in situations where it's not clear why the compilation failed.
Perhaps adding a feature flag to allow 'fallback' mode would be a good alternative?

Makes sense, feel free to close if this is not supported. I don't think a feature flag for 'fallback' mode would have helped in this case so I would rather not have that -- it's about as easy to set target-cpu in .cargo/config as it is to set a Cargo flag.
A reasonably friendly option would be to introduce a deliberate link error if simd isn't available. This way cargo check will work (doesn't need to link) but cargo build won't let you make a non-simd binary. See https://github.com/dtolnay/no-panic for an example. You could do something like:
undefined reference to `

    ERROR[simd-json]: simd support is required; set environment variable RUSTFLAGS='-C target-cpu=native'
    '

I like that idea!

@sunnygleason since you did the sse4.2 work I'm curious on what your take is? Should building unoptimised versions be allowed?

Those are the intrinsics I found in @lemire's arm64 implementation

[x] vaddq_s8
[ ] vandq_u8
[ ] vceqq_s8
[ ] vceqq_u8
[ ] vcgtq_s8
[ ] vcleq_u8
[ ] vdupq_n_s8
[ ] vdupq_n_u8
[ ] vextq_s8
[ ] vget_lane_u64
[ ] vgetq_lane_u16
[ ] vgetq_lane_u64
[ ] vld1q_s8
[ ] vld1q_u8
[ ] vmovq_n_u8
[ ] vmull_p64
[ ] vorrq_s8
[ ] vorrq_u8
[ ] vpaddq_u8
[ ] vqmovn_u64
[ ] vqsubq_u8
[x] vqtbl1q_s8
[x] vqtbl1q_u8
[ ] vreinterpret_u64_u32
[ ] vreinterpretq_s8_u8
[ ] vreinterpretq_u16_u8
[ ] vreinterpretq_u64_u8
[ ] vreinterpretq_u8_s8
[ ] vshrq_n_u8
[ ] vld1q_u8
[ ] vst1q_u8
[ ] vmovq_n_u8
[ ] vceqq_u8
[ ] vandq_u8
[ ] vpaddq_u8
[ ] vgetq_lane_u32
[ ] vreinterpretq_u32_u8

@Licenser I was actually thinking of writing a 'basic' 64-bit implementation, if nothing else than for comparison purposes and general platform ubiquity. Do you like that idea?

Allowing a fallback is as simple as removing the sse4 guard. Rust is smart enough to replace the simd intrinsic with plain x86 code. The drawback is that the he default compiler settings will default to that and give users surprisingly bad experience


feat: add go omitempty json tag

Description

This PR is adding a new option for GO_COMMON_PRESET for generating omitempty parameter.
Related Issue

Checklist

[x] The code follows the project's coding standards and is properly linted (npm run lint).
[x] Tests have been added or updated to cover the changes.
[x] Documentation has been updated to reflect the changes.
[x] All tests pass successfully locally.(npm run test).

Additional Notes

It needs addJsonTag to work, I added an option thinking that if anyone does not need it, if anyone wants to have empty data while Marshaling, they can do it. Just didn't wanted to force the omitempty tag on the user.

The problem as I see it with adding it as an option is that it currently (by default) creates incorrect JSON Data as soon as you have an optional property or null type. To me this is a bug rather then a missing feature 🤔


form_rooted_blob, form_multy_blob; mblob, mBlob - or whatever - instead of form_Blob, Blob

It's confusing memory and breaks the naming convention of the functions. Yes, for a 300-line long code and functions which are next to each other it's not that critical, but in general it's a bad style, and is making search harder when studying the code.
I know it's applied for other variables, but in the other cases it's logically more coherent - single values, aggregated, ... or they reflect significantly different matters like p and P.
https://github.com/boris-kz/CogAlg/blob/9fa47a929ae39acb1d23dd357a7412c3d28f0a45/frame_dblobs.py#L216

Yeah... I just changed blob to seg (blob segment) and Blob to blob, that's
more suggestive.
On Mon, Sep 24, 2018 at 8:03 PM Todor Arnaudov<EMAIL_ADDRESS>wrote:

It's confusing memory and breaks the naming convention of the functions.
Yes, for a 300-line long code and functions which are next to each other
it's not that critical, but in general it's a bad style, and is making
search harder when studying the code.
I know it's applied for other variables, but in the other cases it's
logically more coherent - single values, aggregated, ... or they reflect
significantly different matter like p and P.
https://github.com/boris-kz/CogAlg/blob/9fa47a929ae39acb1d23dd357a7412c3d28f0a45/frame_dblobs.py#L216
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/boris-kz/CogAlg/issues/11, or mute the thread
https://github.com/notifications/unsubscribe-auth/AUAXGahwPeesSVnP25-6VS-G0JY-BD3jks5ueXLlgaJpZM4W3pp8
.


Enable local monitoring databases?

Based on the first diagram, it appears that the Call Data Connector can be configured to send data only to the online CQD:
"With Call Data Connector, the Skype for Business Server pushes call data to the cloud service so that you can leverage the Skype for Business Online Call Analytics (CA) and Call Quality Dashboard (CQD) tools, as shown in the following diagram"
Additionally, the second diagram explains an ability to submit data to both Online CQD and premise monitoring:  "Of course, you might want to keep some call quality data on premises."
This implies that there is an alternative deployment for cloud only monitoring and a hybrid solution of on-premise and cloud based monitoring.
However, this article also states that "monitoring must be enabled on the front end pool".
The question: Is there a way to enable the "local Monitoring Databases" in SfB on Premise (i.e. the caching monitoring databases on the frontend), without deploying a Monitoring SQL server backend?  If so, how?

Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

ID: 87ba30d1-a7b1-7b12-66e3-dbd75ee62a47
Version Independent ID: 776dd776-b0b0-3088-6549-ec01b549f1d0
Content: Plan Call Data Connector
Content Source: Skype/SfbHybrid/hybrid/plan-call-data-connector.md
Product: skype-for-business-itpro
GitHub Login: @CarolynRowe
Microsoft Alias: crowe

@tylerwb15 thank you for reaching out and helping improve MS Docs.  Please have a look at this recent article.  Hopefully this helps. https://docs.microsoft.com/skypeforbusiness/deploy/deploy-monitoring/deploy-monitoring

@tylerwb15 Thank you for submitting feedback. We understand that this issue has been resolved.
Please feel free to re-open this issue if there is a specific area of the docs that we can improve or make better. Thank you.


🧑💻 : Refactoring the header of the Home Page

:red_circle: Title : Styling the Header of the Home Page
:red_circle: Enhancement Aim : To enhance the styling of Home Page to better user experience
:red_circle: Changes :  Enhancement of the Header Page
Screenshots 📷

✅ Details to Include When Taking the Issue:
Name : Vansh Garg (Github username: @VanshGarg06)
Participant Role GGSOC-EXT, Hacktoberfest, level 2

Please assign this issue to me @iamrahulmahato as soon as possible under hacktoberfest and gssoc-ext with necessary labels.
Happy Contributing! 🚀
Wishing you all the best on your open source journey. Enjoy! 😎

Someone is already working on it, so do not demotivate raise a new issue


Backup/Restore Polishing

Deployment:

[x] On the storage class field the tooltip warning message should stay

Backup CRD:
Edit Page:

[x] Schedule should be consistent with workload (cron input type)
[ ] Tool tip for encryption config secret's style to make it taller ⇒ Lauren

List:

[x] State/Status ⇒ need to figure out what's the difference and if we should consolidate followup with Rajashree
[x] The table didn't change and the detail page didn't change, not refreshing
[x] Latest-backup ⇒ should be a custom (Backup File Name)
[x] Add 2 columns for the last and next backup dates
[x] Remove Age

Details:

[x]  In the "config" view should be showing everything. Remove the hiding

Restore CRD:
List:

[x] State/status should be consolidated

Details:

[x] check box style should be disable ⇒ lauren
[x] Radio button doesn't show which one is selected ⇒ lauren

Adding some clarity for QA:

[x]  On the storage class field the tooltip warning message should stay

The warning tooltip (and border) for the storage class dropdown disappeared when different storage radio options were toggled between

Backup CRD:
Edit Page:

[x]  Schedule should be consistent with workload (cron input type)

Input field should have a human-readable display of the cron schedule, as seen in Cron Jobs

List:

[x]  State/Status ⇒ need to figure out what's the difference and if we should consolidate followup with Rajashree

Table views for backup and restore both had 'state' and 'status' columns; they should now have 'status' displayed with the styling from the 'state' column

[x]  The table didn't change and the detail page didn't change, not refreshing

backupfilename needed page refresh to show changes when a scheduled backup ran

[x]  Latest-backup ⇒ should be a custom (Backup File Name)

'Latest-Backup' table header needs a space in it/should be changed to: Backup File Name

Details:

[x]   In the "config" view should be showing everything. Remove the hiding

If scheduling/encryption weren't enabled, their radio groups did not appear in view mode

Adding some clarity for QA:

[x]  On the storage class field the tooltip warning message should stay

The warning tooltip (and border) for the storage class dropdown disappeared when different storage radio options were toggled between

Backup CRD:
Edit Page:

[x]  Schedule should be consistent with workload (cron input type)

Input field should have a human-readable display of the cron schedule, as seen in Cron Jobs

List:

[x]  State/Status ⇒ need to figure out what's the difference and if we should consolidate followup with Rajashree

Table views for backup and restore both had 'state' and 'status' columns; they should now have 'status' displayed with the styling from the 'state' column

[x]  The table didn't change and the detail page didn't change, not refreshing

backupfilename needed page refresh to show changes when a scheduled backup ran

[x]  Latest-backup ⇒ should be a custom (Backup File Name)

'Latest-Backup' table header needs a space in it/should be changed to: Backup File Name

Details:

[x]   In the "config" view should be showing everything. Remove the hiding

If scheduling/encryption weren't enabled, their radio groups did not appear in view mode

On 2.5-head - commit id: ac48370

The warning tooltip (and border) for the storage class dropdown stays when different storage radio options are toggled between

Backup CRD

Input field of the cron schedule is as seen in Cron Jobs

List page

State is displayed

latest backupfilename is displayed without refresh for a recurring backup

Backup File Name column is available

2 columns for the last and next backup dates are avalable

Age column is not available

When scheduling/encryption arent enabled, their radio groups appears in view mode in Details page

On 2.5-head - commit id: ac48370

The warning tooltip (and border) for the storage class dropdown stays when different storage radio options are toggled between

Backup CRD

Input field of the cron schedule is as seen in Cron Jobs

List page

State is displayed

latest backupfilename is displayed without refresh for a recurring backup

Backup File Name column is available

2 columns for the last and next backup dates are avalable

Age column is not available

When scheduling/encryption arent enabled, their radio groups appears in view mode in Details page


honour --build-dir and chdir across filesystems

The BUILDKITE_BUILD_PATH environment variable wasn't being honoured - this assumes that one will be provided and works with it. This also adds a "/D' to cd to make sure we change directory across filesystems, in case you have a data drive mounted you want to build in.

Looks great, thanks for the PR! :heart_decoration:


 This cell type
maintains gap junctions with pigmented epithelial cells." [GOC:tfm,
PMID:15106942]

[Term]
id: CL:0002303
name: pigmented ciliary epithelial cell
def: "A ciliated epithelial cell of the retina, this cell type uptakes sodium
chloride and passes it to non-pigmented ciliary epithelial cells."
[PMID:15106942]

Original issue reported on code.google.com by<EMAIL_ADDRESS>on 27 Feb 2012 at 5:41

@dosumis

TICKET: These terms have names that are too broad for their definitions, or
definitions that are two narrow for their names:

id: CL:0002304
name: non-pigmented ciliary epithelial cell
def: "A multi-ciliated cell of the retina that lacks visual pigment and
contributes to aqueous humor by secreting chloride ions. This cell type
maintains gap junctions with pigmented epithelial cells." [GOC:tfm,
PMID:15106942]

[Term]
id: CL:0002303
name: pigmented ciliary epithelial cell
def: "A ciliated epithelial cell of the retina, this cell type uptakes sodium
chloride and passes it to non-pigmented ciliary epithelial cells."
[PMID:15106942]

Original issue reported on code.google.com by<EMAIL_ADDRESS>on 27 Feb 2012 at 5:41

Would you be ok with

[ ] renaming CL:0002304 'non-pigmented ciliary epithelial cell' as 'retinal multi-ciliated non-pigmented epithelial cell'
[ ] adding part_of 'retina'
[ ] editing CL:0002303 'pigmented ciliary epithelial cell' so it's a subclass of 'ciliated epithelial cell' (rather than 'multi-ciliated epithelial cell'; rename it as 'retinal pigmented ciliary epithelial cell'; make it part_of retina
?
Also,
[ ] fix typo in def of CL:0005012 'multi-ciliated epithelial cell' "A ciliated epithelial cell with many cilium." (cilium => cilia)

@dosumis, unless you have other feedback, I will proceed with Paola's suggestions above.

After taking a closer look at this, it appears the inconsistency in the original definitions is causing confusion rather than this being a narrow vs broad issue.
A 'non-pigmented ciliary epithelial cell', as it is referenced in the dbxref annotation, is not a retinal cell. It is part of the ciliary body, which is part of the uvea. These cells do contribute to production of the aqueous humour, but I have not found a reference that describes them as having cilia or being "multi-ciliated".
Similarly, a 'pigmented ciliary epithelial cell' is part of the ciliary body, not part of the retina. They do participate in the production of aqueous humour with the non-pigmented ciliary epithelial cells, but I have not found any reference that describes them as having cilia.
Proposed solution to come....

Additionally:
[ ] fix typo in def of CL:0005012 'multi-ciliated epithelial cell' "A ciliated epithelial cell with many cilium." (cilium => cilia)


[material-ui][Menu] Menu Item custom attribute is not accessible on a select TextField's onChange

Steps to reproduce
Link to live example: (required)
Steps:

import { TextField, MenuItem } from "@mui/material";
2```
.<TextField
select
label="Select Example"
defaultValue="select"
name="select-example"
id="select-example"
onChange={(event) => {
console.log(event.target.selectedOptions[0].getAttribute("customattribute"));
}}
>

3.`<MenuItem value="select" customattribute="test value" >Select example</MenuItem>`

### Current behavior

The customattribute is not accessible
if we add any custom attribute to any menu item it should be accessible on onChange the same way  it is accessible in normal html

### Expected behavior

the custom attribute of <MenuItem value="test" customattribute="custom attrubute test"> should be accessible in on change of `<TextField select   onChange={(event) => {
       console.log(event.target.value); // select
       event.target.selectedOptions[0].getAttribute("customattribute"); // undefined
   }}>`
``

### Context

i need to access two values from a menu item, get the values on onChange and make an api call which requires both of the values, but with the current situation there is no way to do so, i alos tried to pass values on object, but that is giving more issues

please fix it, because of this minor issue people choose to go with other option other than Mui

### Your environment

  npx @mui/envinfo

Browser : Chrome
System:
OS: Windows 11 10.0.22631
Binaries:
Node: 18.17.1 - C:\Program Files\nodejs\node.EXE
npm: 10.0.0 - C:\Program Files\nodejs\npm.CMD
pnpm: Not Found
Browsers:
Chrome: Not Found
Edge: Chromium (122.0.2365.52)
npmPackages:
@emotion/react: ^11.11.1 => 11.11.1
@emotion/styled: ^11.11.0 => 11.11.0
@mui/base:  5.0.0-beta.28
@mui/core-downloads-tracker:  5.15.1
@mui/envinfo: ^2.0.18 => 2.0.18
@mui/icons-material: ^5.15.1 => 5.15.1
@mui/lab: ^5.0.0-alpha.159 => 5.0.0-alpha.159
@mui/material: ^5.15.1 => 5.15.1
@mui/private-theming:  5.15.3
@mui/styled-engine:  5.15.3
@mui/system:  5.15.3
@mui/types:  7.2.12
@mui/utils:  5.15.3
@types/react:  18.2.45
react: ^18.2.0 => 18.2.0
react-dom: ^18.2.0 => 18.2.0
typescript: ^4.9.5 => 4.9.5

**Search keywords**: MenuItem, TextField

It doesn't look like this bug report has proper info for one of us to reproduce it.
Please provide a Stackblitz (https://stackblitz.com/edit/stackblitz-starters-maxhor), a link to a repository on GitHub, or provide a minimal code example that reproduces the problem.
Here are some tips for providing a minimal example: https://stackoverflow.com/help/mcve.


OSX compatibility with grpc 0.13 integration

Thanks for the fixes :) Let's hold off on it shortly and and get my gRPC Python installation fixes in first: https://github.com/youtube/vitess/pull/1541
It's not done yet as some platforms are still broken. Once it's done, I'll let you know. After that, can you please adjust your PR?

@michael-berlin sure, no problem, I pushed the adjustment

https://github.com/youtube/vitess/pull/1541 is in now.
Can you please rebase your change on that one?
After that, I'm happy to merge your change.

Did you mean like this? ^^

No. It looks like you cherry-picked my temporary commits now.
Instead, can you just rebase your commit on the latest master?
I'm not sure how much familiar you're with git, but I would probably do it this way:
git rebase -i HEAD~10

This will represent you the list of the 10 last commits. Delete all "WIP: ..." ones and leave only yours.
After that, rebase your change on top of the current master:
git rebase master
(assuming that "master" points to a recent local branch/copy of the master branch from github.com/youtube/vitess)

Well, I know how to rebase, I just didn't understand on top of what I was supposed to rebase :) Is this ok now?

Sorry for not being clear enough :)
Did you test it on your OSX machine? Does this work? Once you confirmed, I'll merge it.

Yes, I just rebuilt all the dependencies, works fine.

LGTM


Is there a more-friendly-to-parsing parseable version of the spec?

I've been parsing the spec recently to auto-generate classes to use for an LSP server implementation, but found some little quirks that made it more complicated than it could've been.
I wondered whether there was another version of the spec besides the markdown one to aid code-generation of interfaces for other languages? I wonder how other projects are keeping up with the latest changes easily?

I imagine some people generate stuff from the TypeScript code from the Microsoft/vscode-languageserver-node repository.
Also see #67.


Problem with slashes using contains

When I try to use Where like
var files = new[]
            {
                new SomeFileModel { Name = "a\\b" },
                new SomeFileModel { Name = "c" },
                new SomeFileModel { Name = "a" },
            };
string filter = "name.contains(\"\\\")";
var result = files.AsQueryable().Where(filter).Select(x => x.Name).ToList();

I get an error System.Linq.Dynamic.Core.Exceptions.ParseException: 'Unterminated string literal'.
Sounds reasonable as the result filter is "\". But when I try to use this
string filter = "name.contains(\"\\\\\")";
The result is empty. In this case filter doesn't work.  But if I use
string filter = "name.contains('\\')";
it works fine. How can I do search for strings with slashes?

String parsing is / escaping logic is not 100% correct I think.
For now use:
string filter = "name.Contains(@0)";
var result = files.AsQueryable().Where(filter, "\\").Select(x => x.Name).ToList();

I think I found the problem in your parsing. I can't see the reason to do this in string literal parsing
if (next == '"')
{
     NextChar();
}

Because if I use query "contains(\"\\\")" it fails with unterminated literal, but if I change query to "contains(\"\\a\")" it works finding "\a" string. And also if I use "contains(\"\\\\\")" it works with string "\\" (string with two real slashes).


Replace our own implementation of lzo by decompress.lzo

decompress.lzo implements also a compressor which can help about the implementation of a VPN server. Tests are kept to check that everything works even with the replacement. The directory minilzo is removed (because unused) and lzo_fuzzable.ml was updated even if decompress.lzo has its own fuzzer.

Currently, we use the upstream version of decompress but a release can be done (a minor one, the interface does not change at all!).

Thanks. Indeed I prefer to have a decompress release and specify here in opam only the lower bound :)

Thanks! The change seems straight forward and with decompress released I think this is safe to merge.


Opentyrian doesn't work

Hello, I have installed a couple of ports and they run fine, for example Celeste Classic.
But now I installed OpenTyrian, the installer reported success but there is no entry for the game under Ports. I did restart EmulationStation and then the unit itself, I still don't see it. Any ideas?
Also, where are these ports located? Which directory?
Using an rg503 on the latest version as of today.

Where is the ports folder exactly?

in the easyroms(roms) partition.  Same as other systems like nes and gba.

Any update on this?  Also, are you installing the ports via the portmaster tool located in the options/tools section?

Reading the instructions for the portmaster tool shown here https://github.com/christianhaitian/PortMaster#install-info


Add a license to the repository

I recently came across your project Quantum-Computing and I noticed that the repository github.com/tinegachris/Quantum-Computing currently does not have a license specified.
Why Adding a License is Important?
As GitHub documents

Public repositories on GitHub are often used to share open source software. For your repository to truly be open source, you'll need to license it so that others are free to use, change, and distribute the software.

Would be possible to add a valid license to this repository? Probably something really OpenSource such as the Apache 2.0 license would fit really well.
Thanks!

Hi @cbjuan I have done this. Thanks for the heads-up.


feat(Testing): add movement simulator resolves #262

For many trivial tests one still has to put on the headset in order to
move to the right spot. This increases turn-around times. This script
can be put onto the camera rig and will then allow movement through
the scene using keys. Once at the desired location the controllers can
be used to interact while looking at the monitor.
Supported so far are move, strafe, rotate and up/down. Being an early
version this is only tested in one context so far and especially after
rotating the move directions are inverted currently. These are all
things to look at in future versions. Help always welcome.

Works as expected.
I'd suggest though the script lives in the main scripts folder and not the Helper folder. I'm trying to make it so the Helper folder is just classes that store common functions or hidden away stuff that developers dont need to worry about.
Also worth adding some docs on this to the new DOCUMENTATION.md file.

resolves #324

New patch pushed

are you happy for this to be merged now? Looks like a decent enough start on the feature.

yep


Can't compile when using a class enhanced with Lombok

Hi!
Thanks for this amazing plugin, thanks to it I can use Dagger 2 in a pure java module :)
But I've found a problem using it with [Lombok]
(https://projectlombok.org).
I have declared this class:
@AllArgsConstructor(access = AccessLevel.PROTECTED)
@Data
public class UserDemoEntity {
private final int id;
private final String login;
}
But when I try to compile the module I get this error:
error: variable id not initialized in the default constructor
error: variable login not initialized in the default constructor
If I try to use the generated setter by Lombok for the attributes in any class, I get this error too.
Error: cannot find symbol method getId()
It seems like the plugin I’ve installed for Intellj in order to support Lombok annotations crashes when using this plugin.
But the "structure window" of the Ide still shows the autogenerated setter/getters etc

I'm using Android Studio by the way.
Thanks!

As far as IntelliJ is concerned, the gradle-apt-plugin only configures your apt and compileOnly (resp. for tests) as compile dependencies in IntelliJ so that you can simply enable annotation processing in IntelliJ and annotations are processed from within the IDE. So you should try creating a project that does not use the gradle-apt-plugin and simply puts your Dagger dependencies at compile scope, and see if you reproduce the issue.
Lombok is a clever hack, but an hideous hack anyway, and I strongly believe no sane person should be using Lombok. There are non-hackish alternatives, even if they're not always automated at the same level (at a minimum, use delombok to generate plain Java sources so that you don't need to inject Lombok's hacks everywhere in your toolchain).
In any case, I believe the issue is with the lombok-intellij-plugin, not the gradle-apt-plugin or anything else.

Hi!
I've configured my project as you suggested:
"So you should try creating a project that does not use the gradle-apt-plugin and simply puts your Dagger dependencies at compile scope, and see if you reproduce the issue".
And when I run the task compileJava, I don't get any error. But as soon as I "use cradle-apt-plugin", I get the error already commented.

OK, let me try to reproduce then. How's Lombok configured? Maybe you can share snippets of your gradle.build?

Note that I still believe the bug is not in gradle-apt-plugin. Also, is the error only in IntelliJ/Android Studio or (also?) when building with Gradle? (it's not very clear from your reports)

The error happens too when building with Gradle.
Top level build project
https://gist.github.com/VictorAlbertos/8b57b5240babc8863bb2
Build file for the specific module called "domain".
https://gist.github.com/VictorAlbertos/1e14d6047f511ebba508#file-build-module-gradle
Here some screenshots of the output generated when trying to run the task compile java for the domain module:

And the class enhanced with lombok:

Because I'll never install an IntelliJ plugin for Lombok, I tested in pure Gradle, in three flavors:
First with everything in the compile scope:
apply plugin: 'java'

repositories {
  mavenCentral()
}
dependencies {
  compile 'com.google.dagger:dagger-compiler:2.0.2'
  compile 'org.projectlombok:lombok:1.16.6'
}
compileJava {
  outputs.dir 'gen/'
  doFirst { mkdir('gen/') }
  options.compilerArgs += [ '-s', 'gen/' ]
}

Then with an apt configuration and using the -processorpath (more or less what the gradle-apt-plugin does):
apply plugin: 'java'

repositories {
  mavenCentral()
}
configurations {
  apt
}
dependencies {
  compile 'com.google.dagger:dagger:2.0.2'
  apt 'com.google.dagger:dagger-compiler:2.0.2'
  compile 'org.projectlombok:lombok:1.16.6'
  apt 'org.projectlombok:lombok:1.16.6'
}
compileJava {
  inputs.files configurations.apt
  outputs.dir 'gen/'
  options.compilerArgs += [ '-s', 'gen/', '-processorpath', configurations.apt.asPath ]
  doFirst {
    mkdir('gen/')
  }
}

And finally with the gradle-apt-plugin:
buildscript {
  repositories {
    maven {
      url "https://plugins.gradle.org/m2/"
    }
  }
  dependencies {
    classpath "net.ltgt.gradle:gradle-apt-plugin:0.4"
  }
}

apply plugin: "net.ltgt.apt"
apply plugin: 'java'

repositories {
  mavenCentral()
}
dependencies {
  compileOnly 'com.google.dagger:dagger:2.0.2'
  apt 'com.google.dagger:dagger-compiler:2.0.2'
  compileOnly 'org.projectlombok:lombok:1.16.6'
  apt 'org.projectlombok:lombok:1.16.6'
}

In all cases this compiles without error. As source files, I used your class from above, a dummy Dagger @Module class to trigger the Dagger annotation processor (the options.compilerArgs above are used to get the generated Module_…Factory class and thus verify the annotation processor is correctly triggered), and a Main class the instantiates your class and calls its getters (i.e. makes use of Lombok-generated constructor and getters).

Thanks for the support!
The problem was fixed adding this line to my build gradle module domain (I saw it in your snippet):
apt 'org.projectlombok:lombok:1.16.6'
But if I don't use your plugin the task compileJava works fine. I don't know why this happens, but my knowledge of gradle is pretty limited.

gradle-apt-plugin (and the android-apt plugin I believe, see https://projectlombok.org/setup/android.html) puts the apt dependencies in the processor path of javac (see second try above) so there's a clear separation between the dependencies your code can rely on and the ones depended on by the annotation processors.
See https://docs.oracle.com/javase/8/docs/technotes/tools/unix/javac.html for more information about -processorpath.


thresholds display in scientific format

When I run View(thresholdMWR), the Fresh_1 thresholds display in scientific format and the Marine_1 thresholds display with 5 decimals. Is it possible to make all of the thresholds display numerically? Two decimal places would be fine for everything.

@ben-wetherill all those columns are numeric, so they are displaying numerically, but the presentatation (either significant digits or scientific notation) is defined by the one value in a column that has the greatest number of significant digits.  They all display the same if I round them to two decimal places.  The file has to change to do this, I don't think there's a way to do this strictly with View().

I see they are displaying with 2 or 1 digits now, so that is better.


Add abillity to upload images other than 1:1 aspect ratio. Allow pngs.

Allow any aspect ratio images to be uploaded. It will add black borders to the sides
Allow any file type upload. Fixes #13

Please review and merge if all looks good. I'm not a python developer so please review the code and suggest any improvements.

Yeah, for me it's fine. I will inspect it with more attention soon when I release a new version of this library.
Thanks man! 👍


Redundant variables

There's a few variables that should be constants instead of being redefined in every function e.g.
https://github.com/TomCasavant/MastodonPlaysGameboy/blob/45f30df961011da878cb48fc447a3aee789e9ff6/gb.py#L257

Hi Tom, I'd like to work on this issue. Is there anything I'd need to download or install besides the repo itself?

Hi Tom, I'd like to work on this issue. Is there anything I'd need to download or install in order to test my changes, besides the repo itself?

When you clone the repo you'll want to do pip install -r requirements.txt and then set up your config file, unfortunately for legal reasons I can't provide you with a rom file so you'll have to source one yourself.
If you want to test the mastodon side you'll need a bot account on a mastodon server (if your server doesn't support more than 4 poll options you'll have to work off of @phi1997's pull request which hasn't been merged in yet #33. Fortunately for this issue you probably won't need a mastodon account, just uncomment https://github.com/TomCasavant/MastodonPlaysGameboy/blob/45f30df961011da878cb48fc447a3aee789e9ff6/bot.py#L326 and comment out the following line
Worst case, if you just want to make the updates and put them in a pull request I'll go ahead and test them for you

Is this the right place to enter that pip command? (I redacted my computer name)

Is this the right place to enter that pip command? (I redacted my computer name)

Yeah that's where you'd run it from (https://note.nkmk.me/en/python-pip-install-requirements/),
Are you on Windows? I haven't tried it yet, so I'm not 100% certain all the packages work there.
Also, if you're new to this I'd also recommend setting up a virtual environment (https://docs.python.org/3/library/venv.html) and you'll want to make sure you're running a python version >3.9
If you're not familiar a virtual environment is pretty easy to setup if you have python installed already:
python -m pip install virtualenv
python -m  virtualenv .venv # Creates a virtual environment in the .venv directory
.\.venv\Scripts\activate # Starts the virtual environment (in linux this would be source .venv/bin/activate)
And then when you're in the virtual environment install all the packages with the pip install -r requirements.txt command

I've made script_dir constant by moving it above the class, but I'm not sure I can do the same with screenshot_dir and gif_dir because they depend on directory and image_path respectively. Should I go ahead with the pull request, or am I missing something?

Okay, I finally got around to working on this. So far I've made script_dir constant by moving it above the classes, but I'm not sure I can do the same with screenshot_dir and gif_dir because they depend on function parameters. Should I go ahead with the pull request, or am I missing something? Also, which branch should I be working on?

Yeah I might change how screenshot_dir and gif_dir work later on but that's fine for now could you move save_dir as well I'm pretty sure that's constant throughout the project? And then yeah that should be good for a PR

I think you meant save_loc, so I've moved that, ids_loc, and gif_dir (turns out I could move it in bot.py), but I still need to know which branch to put the PR on

Just the main branch is fine


Xadgroup/feature/membersvalidation

Removed parameter checks for Members, MembersToInclude and MembersToExclude as per Issue #116.
All AppVeyor tests are passing after the changes.

Hi @rcarpenter79, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@rcarpenter79, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.
Thanks, MSBOT;

Hello, @rcarpenter79.
I am trying to understand the motivation for this change. Would you give us a sample of #116?
Thanks!

Hi @narrieta
I have uploaded a sample file containing the use case that we're using.
The aim is to keep the data and configuration separate.
We re-use the configuration between environments but vary the data and the number of users and groups will vary.
Thanks.
xADGroup Members Example #116.txt

@rcarpenter79 I think I see what you're trying to do. We overcome this internally by using the equivalent of this:
Configuration Example
{
    Import-DscResource -ModuleName ActiveDirectory

    Node ExampleDC
    {
        # Active Directory Setup here...

        foreach($User in $Node.Users)
        {
            xADUser $User.UserName
            {
                Ensure                          = $User.Ensure
                UserName                        = $User.UserName
                DomainName                      = $Node.DomainName
                DependsOn                       = '[xWaitForADDomain]DscForestWait'
                Password                        = (Import-Clixml -Path ('C:\Credentials\{0}.clixml' -f $User.UserName) )
                DomainAdministratorCredential   = (Import-Clixml -Path ('C:\Credentials\DomainAdmin.clixml') )
                PasswordAuthentication          = 'Negotiate'
            }
        }

        foreach($Group in $Node.Groups)
        {
            if ($null -ne $Group.Members)
            {
                xADGroup $Group.GroupName.Replace(' ', '')
                {
                    Ensure              = $Group.Present
                    GroupName           = $Group.GroupName
                    Description         = $Group.Description
                    Members             = $Group.Members
                    GroupScope          = $Group.GroupScope
                    Path		        = $Group.Path
                    Notes		        = $Group.Notes
                    Credential	        = (Import-Clixml -Path 'C:\Credentials\DomainAdmin.clixml')
                    DependsOn           = $Group.DependsOn
                }
            }
            else
            {
                xADGroup $Group.GroupName.Replace(' ', '')
                {
                    Ensure              = $Group.Present
                    GroupName           = $Group.GroupName
                    Description         = $Group.Description
                    MembersToInclude    = $Group.MembersToInclude
                    MembersToExclude    = $Group.MembersToExclude
                    GroupScope          = $Group.GroupScope
                    Path		        = $Group.Path
                    Notes		        = $Group.Notes
                    Credential	        = (Import-Clixml -Path 'C:\Credentials\DomainAdmin.clixml')
                    DependsOn           = $Group.DependsOn
                }
            }
        }
    }
}

@narrieta Wouldn't it be handy to be able to use splatting against DSC resources inside of configurations? We use this "work around" all the time, but it does make for a lot of extra unnecessary code.

@iainbrighton I considered your example first. The problem I saw with this is the resource configuration in the else block is only valid when MembersToInclude and MembersToExclude aren't empty. There are three other permutations that could be specified in the data file when either or both are empty.
Splatting would be great though.

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable


Add context argument to Querier.Select/ChunkQuerier.Select

In order to facilitate adding a context argument to among others storage.LabelQuerier.LabelValues and tsdb.PostingsForMatchers, move the context argument from storage.Querable.Queryable.Querier/storage.Querable.ChunkQueryable.ChunkQuerier to storage.Querier.Select/storage.ChunkQuerier.Select.
In any case, it should be more natural/idiomatic to pass context as a function argument, rather than a struct field. See also @bboreham's comment to the same effect. This should be a pure refactoring, without any behavioral changes.

Putting this in draft mode for the time being, while I work on my other changes on top of this PR.

Thanks for the review @bboreham - I fixed the ones I could find (i.e., re-use ctx in tests).

The failing test looks like a flake.

Rerunning failed jobs.


fix: Update NAC CRDs

Why the changes were made
Update NAB CRD for https://github.com/migtools/oadp-non-admin/pull/120
How to test the changes made
Follow NAC development documentation.

/retest

/test 4.16-e2e-test-aws


Make log-dir accessible for random UIDs

In OpenShift (DevSpaces/Che) container assigns a random UID. So make log-dir accessible for it when Kafka tries to create files.

Thank you @xstefank it works as expected!


ci: Add github actions

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@73c3417). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             main        #1   +/-   ##
========================================
  Coverage        ?   100.00%
========================================
  Files           ?         3
  Lines           ?        72
  Branches        ?         0
========================================
  Hits            ?        72
  Misses          ?         0
  Partials        ?         0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73c3417...1aaeac7. Read the comment docs.


Fixing the way Selector handles SSL connection handshakes

Currently, for connections over SSL, the ambry selector returns an established connection after a poll even before the handshake is completed. This is not very useful as the caller will then have to repeatedly check outside of the selector poll on whether the handshake is complete, which is not ideal.
This patch fixes this issue.
The selector will only return a connection after the connection is established and the handshake is complete. If the handshake does not complete for any reason, then it should be treated as a connection establishment failure. This way the callers of the selector can be agnostic to SSL, and can interact with the selector in a completely non-blocking way.
SLA: 10 mins
Reviewers: Priyesh, Ming

Done with a pass. Let me know when you update the PR.

Left a few more minor comments. The rest LGTM.

LGTM.

@pnarayanan @vgkholla : done addressing the comments.

Not related to your patch, but I just noticed that PlainTextTransmission constructor does not initialize the logger correctly (It initializes it with SSLTransmission.class). We should fix this asap, could you make the change as part of this patch?

@pnarayanan : done addressing all your comments.

@pnarayanan : done fixing the last ones.

The selector exposes public boolean isChannelReady(String connectionId) which can be used to check whether a connection is ready to be added to the connected list directly. This means that you can simplify this code considerably and get rid of the UnreadyConnections class and the map. You will most likely only need a set of connection ids which you can iterate over. You don't need to save the transmissions.
While at it, please also get rid of any variables in the newly added code that assumes SSL (those that refer to handshakes, for example). I don't think you will need to refer to transmissions, either.

"selector exposes" - I should say selector has such a method (whether it exposes it is irrelevant as all this code is within the selector).

@pnarayanan : yep, I don't know how I missed that :(. Have fixed it.

I am just throwing some wild thought for discussion. Instead of current way to passively check if handshake is done, would it be possible to pass a callback to an SSLTransmission, so that in handshakeFinished() it becomes:
if (!handshakeComplete) {
    key.interestOps(key.interestOps() | SelectionKey.OP_WRITE);
} else {
      key.interestOps(key.interestOps() & ~SelectionKey.OP_WRITE);
      metrics.sslHandshakeCount.inc();
      metrics.sslHandshakeTime.update(time.milliseconds() - handshakeStartTime);
      callback.onComplete() // move the connectionId to the connected list.
}

This may help us save unnecessary polling cycles.

Selector should and does all its work in poll. Even if we did have a callback update the status of the connection, that connection will only get returned after a poll, so it is pointless.

@nsivabalan I think you can avoid all references to handshake in this patch after removing the metric. Please make sure you do that. Thanks.

@pnarayanan @xiahome I knew that we already have a metric to track handshake time and hence named this as sslPerceivedHandshakeTime. One of you guys did even ack me verbally that it would be good to have this in place. Anyways, is there any more comments you want me to address. I will push in one go. I wish to push this in today.

@nsivabalan regardless of the previous context, what value does sslPerceivedHandshake provide that the other metric doesn't provide?

Just to track the difference between when selector moves the connection to connected list and when the actual handshake completes. Lets say for some reason, handshake was over in x secs, but then selector took x+y secs to move the connection to connected state and if y is in order of secs and not ms. Ideally it shouldn't happen, but just incase.

I don't think that adds much value. Why do it just for handshakes then? You could make the same argument about actual connection time and the time at which the connection gets added to the connected list (for plain text).
I know I agreed to this before, but at the time I thought handshake time was not getting accounted for in any other metric.

Plus, how long selector takes to add something to its list is not under the control of selector at all - it depends entirely on how often the caller polls it and how long the allowed poll time is and all that. So this metric would really not belong to the selector anyway.

yep, I can get you. I have removed it. Updated the patch

@xiahome @pnarayanan : can one of you guys merge if it looks good

@nsivabalan I don't think you have updated the patch.

@pnarayanan just told me that this requires some more work from @nsivabalan. Holding off reviewing until that is resolved.

@pnarayanan : can you tell me what are the comments yet to be addressed?

"... I have removed it. Updated the patch" - what have you removed?

@pnarayanan I think @nsivabalan  meant the duplicate metric for ssl handshaking time is removed.

@pnarayanan yes, I meant the same. That I have removed the duplicate metric

@xiahome @pnarayanan @vgkholla : I have updated the patch.

Once again, selector should not refer to handshakes. Handshakes are internal to SSLTransmission. As far as the Selector is concerned, connections are ready or unready.
Your changes therefore should not make references to "handshakes" in the Selector code. And like I said in the other comment, the other metric does not see useful, so I would think you wouldn't have to refer to "handshakes" anywhere in the patch.
p.s: The current selector code has references to handshakes, but only in comments (and that is wrong).

LGTM

Two very minor comments. LGTM otherwise.
It looks like the tests cover the new code but could you add coverage details in the description and confirm that the new code is fully covered (including true and false for any conditionals that exist)?


Add flocking behaviour

As addition to the social force model, add flocking behaviour perhaps inspired by http://dl.acm.org/citation.cfm?id=37406

closing this until we get more hands on deck


The number inputs in the editor should be changeable with arrow keys

In the cropper view, you can change the number inputs with arrow up/down keys, and you can also press Shift to increase/decrease by 10. Would be nice if this worked in the editor window too, for the width, height, and FPS inputs. Should probably make it a reusable component.

@sindresorhus mind if I try tackling this one?

@bringking Go ahead :)


rerun: 0.13.0 -> 0.17.0

Description of changes

the patches are not necessary anymore
the repository now exposes other build targets, so we need to specifically build the rerun-cli package with native_viewer feature
this crate was also hit by this issue with rust 1.80, the crate itself ships with a rust-toolchain file which states which rust version they use upstream, so I went ahead and build the crate with a non standard rustPlatform (1.76 instead of 1.80 currently, but the toolchain now updates with the src)

Things done

Built on platform(s)

[x] x86_64-linux
[ ] aarch64-linux
[ ] x86_64-darwin
[ ] aarch64-darwin

For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)

[ ] sandbox = relaxed
[ ] sandbox = true

[ ] Tested, as applicable:

NixOS test(s) (look inside nixos/tests)
and/or package tests
or, for functions and "core" functionality, tests in lib/tests or pkgs/test
made sure NixOS tests are linked to the relevant packages

[ ] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
[ ] Tested basic functionality of all binary files (usually in ./result/bin/)
24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)

[ ] (Package updates) Added a release notes entry if the change is major or breaking
[ ] (Module updates) Added a release notes entry if the change is significant
[ ] (Module addition) Added a release notes entry if adding a new NixOS module

[ ] Fits CONTRIBUTING.md.

Add a :+1: reaction to pull requests you find important.

   error: undefined variable 'RobWalt'

Do you need to add yourself to maintainers.nix?  (If so, please do so in a separate commit, before the current one.)

   error: undefined variable 'RobWalt'

Do you need to add yourself to maintainers.nix? (If so, please do so in a separate commit, before the current one.)

I'm sorry for all the little nits you needed to comment on. It was just robwalt instead of RobWalt 🤦🏼

@ofborg build python312Packages.rerun-sdk

rerun-cli: 0.13.0 -> 0.17.0

The name of the package in nixpkgs is rerun, that should reflect in the commit message in order to trigger Ofborg. It seems like upstream has changed the target name in their cargo file? I'm not opposed to renaming the attribute to rerun-cli instead of rerun (with an alias), although I feel like that'd be unnecessary friction

rerun-cli: 0.13.0 -> 0.17.0

The name of the package in nixpkgs is rerun, that should reflect in the commit message in order to trigger Ofborg. It seems like upstream has changed the target name in their cargo file? I'm not opposed to renaming the attribute to rerun-cli instead of rerun (with an alias), although I feel like that'd be unnecessary friction

I think in the end it's just the package name that changed. The binary is still named rerun. I'll change the title of the PR

I'll change the title of the PR

ok by me but Ofborg looks at the commit message not at the PR title

@SomeoneSerge I think it ran through now successfully 🤠

Hash mismatch on OfBorg.

Yes, my presumption still is that it's https://github.com/NixOS/nixpkgs/pull/334854#discussion_r1720046595

Took me a while but the passthru tests now build locally and are all green. Thanks to you two I learned a lot!
@SomeoneSerge FYI

I didn't do much thorough testing but nix run github:RobWalt/nixpkgs/rerun-0.17#rerun works as expected. Thank you @RobWalt!


New gem owner

Hi @rhomeister ,
I've added you as an owner for this gem on rubygems.org. That way you can also make new gem releases.
You made a lot of quality contributions and it's been great working with you overall.

I'll just assume this PR doesn't need code review :wink:

@bruno- Hehe, you have my blessing on this one. Feeling it mutual by the way!


Enforce consistency with commas in lists

This was originally disabled without much explanation. In fact, the
suggested issue doesn't actually appear to be a bug i.e. the Cop was
disabled in error. We should re-enable the Cop (now split into three
separate Cops) in order to reduce the inconsistency that has since
spread across apps on GOV.UK. Having the flexibility to make this
decision is unnecessary and just takes up more development time.
Original PR: https://github.com/alphagov/styleguides/pull/50

Enforcing trailing commas on every list line is good for diffs as an entire preceding line doesn't change just because someone added a new line to the bottom of a list. :+1: from me, especially if humans don't have to be linters any more.

I like the idea of switching this on, but is there an estimate of the number of lines of code across GOV.UK that would be affected by switching this on (i.e. would the next person to merge to each app need to make a large number of code changes)?

I'd hope that the "only lint the diff" setting would stop the "massive code changes" problem and just cause us to change them incrementally when someone touches a file? But it would be good to estimate that, agreed.

I'm happy with this, it didn't seem to cause any arguments of frustrations in the year we trialled it with Content Publisher as far as I'm aware.
Only thing that was a bit annoying was remembering it in deeply nested hash/array collections - but they should be discouraged anyway.
Is it a change rubocop can auto-correct for us?

@kevindew @brucebolt I believe this is something we can auto-correct across the apps, and as with https://github.com/alphagov/finder-frontend/pull/1545, the intention is to apply it across all files for each app, which I'm planning to help with. This will help avoid weird diffs subsequently. I think it's a good idea to trial this in a couple of apps first 👍:

finder-frontend
content-tagger
whitehall

Do those sound like a good selection? I'd be looking for anything that couldn't be auto-corrected, or which (somehow) causes the tests to start failing.

@brucebolt @issyl0 @kevindew I've now tested the net change across those three apps, and auto-correct worked in all cases, without affecting the tests. The biggest change (surprise, surprise) was Whitehall, with ~12,000 corrections: https://github.com/alphagov/whitehall/pull/5034.
Does that give everyone the reassurance they were looking for?

@brucebolt @issyl0 @kevindew I've now tested the net change across those three apps, and auto-correct worked in all cases, without affecting the tests. The biggest change (surprise, surprise) was Whitehall, with ~12,000 corrections: alphagov/whitehall#5034.
Does that give everyone the reassurance they were looking for?

Sounds good to me 👍

@brucebolt @issyl0 @kevindew I've now tested the net change across those three apps, and auto-correct worked in all cases, without affecting the tests. The biggest change (surprise, surprise) was Whitehall, with ~12,000 corrections: alphagov/whitehall#5034.
Does that give everyone the reassurance they were looking for?

That sounds good to me 👍

Don’t suppose anyone wants to frame their latest comments in an approval kind of way? ;-)

I think it would be a good idea to auto-correct the linting issues that will arise in our most commonly used apps before we switch this on

@brucebolt I can look into that. However, I'd like to try and keep the intent (this PR) separate from the work, if possible. I'm worried that I'll get bogged down in persuading people to review lots of PRs on different apps, which is likely to be a slow process when there's no central motivation to change (yet). Just to restate: I'm happy to help out on the individual apps when the change goes in.
On the other hand, do you think we'll learn something from doing more pre-work? If so, it'd be helpful if you could recommend which apps we'd consider as the most commonly used.
It may help to reiterate that we already trialled these changes: https://github.com/alphagov/content-publisher/pull/169

@kevindew just to recap the discussion we had...agreed next steps are:

Merge this in and do a major release
Wait for the dependabot PRs
Script an extra autocorrect commit on those PRs

One piece of knowledge I was missing is that most of our repos no longer use diff-based linting, so there's a low chance of someone merging in the PR and suddenly getting sporadic linting issues - in any case, we'll still be looking to script a fix across all repos.
@brucebolt I'm going to merge this now, but let me know if you still have concerns about the plan here.