Page MenuHomePhabricator

efl.pack_linear: Clarify behavior and docs
ClosedPublic

Authored by segfaultxavi on Mar 20 2019, 9:48 PM.

Details

Summary

Some APIs accept both positive and negative indices when accessing items.
This patch changes the documentation for the lower limit from -(count - 1) to
-count to allow accessing the very first item.

For example (content_count = 5):

first itemlast item
positive index01234
negative index-5-4-3-2-1

If negative indices are limited to be >= -4 the first item cannot be accessed
using negative indices.

Also, range limit of pack_at is removed for usability.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
YOhoho created this revision.Mar 20 2019, 9:48 PM
YOhoho requested review of this revision.Mar 20 2019, 9:48 PM
segfaultxavi requested changes to this revision.Mar 21 2019, 7:24 AM

In the other ticket I meant that you could create the task and I would write the docs, but anyway, good job!

src/lib/efl/interfaces/efl_pack_linear.eo
63

BEFORE or AFTER the item in the specified index?

65–71

To make it extremely clear I would say:

$index ranges from -$count to $count-1, where positive numbers go
from first item (0) to last item ($count-1), and negative numbers go
from last item (-1) to first item (-$count). Where $count is the number
of items currently in the container.

If $index is less than -$count, it will trigger @.pack_begin($subobj)
whereas $index greater than $count-1 will trigger @.pack_end($subobj)

I removed the example because I am not sure inserting at index -1 (or index count-1) is the same as pack_end(). Are you sure of this? When there is only one item in the list (count=1), you can only insert at position 0. This means BEFORE or AFTER the item already in the list?

78–81

This index parameter needs the same docs as the other indices.

93–94

I would use here the same paragraph I suggest above.

112

Does this allow negative indices too? Then is has to be explained.

This revision now requires changes to proceed.Mar 21 2019, 7:24 AM
YOhoho added inline comments.Mar 22 2019, 2:07 AM
src/lib/efl/interfaces/efl_pack_linear.eo
63

It should be BEFORE. in AFTER case, $subobj index will be $index + 1.
obj.pack_index_get(obj.pack_at(subobj, N)) have to return N, when (0 <= N <= count).

65–71

It should be BEFORE. in AFTER case, $subobj index will be 1.
obj.pack_index_get(obj.pack_at(subobj, 0)) have to return 0.

YOhoho updated this revision to Diff 20861.Mar 22 2019, 2:08 AM

Thank you for suggestion

YOhoho added inline comments.Mar 22 2019, 2:16 AM
src/lib/efl/interfaces/efl_pack_linear.eo
63

Oops,

obj.pack_at(subobj, N);
obj.pack_index_get(subobj);
65–71
obj.pack_at(subobj, 0);
obj.pack_index_get(subobj);
segfaultxavi added inline comments.Mar 22 2019, 6:12 AM
src/lib/efl/interfaces/efl_pack_linear.eo
63

OK, in this case, the above sentence could be:

Inserts $subobj before the item at position $index.
YOhoho updated this revision to Diff 20896.Mar 22 2019, 1:23 PM

update docs

This revision is now accepted and ready to land.Mar 23 2019, 1:41 AM
bu5hm4n added a subscriber: bu5hm4n.Apr 2 2019, 8:18 AM

The commit message should not say enhance documentation, but rather redefine. as the behavior of pack_at is changed, before the index specified the position where the new element would be, hence the sentence about -1 would imply that the element is added as last. The new documentation changes this behavior.

src/lib/efl/interfaces/efl_pack_linear.eo
71

Similar semantic descriptions should be added at pack_at and any function that takes a index.

Fullstop at the end is missing.

bu5hm4n requested changes to this revision.Apr 2 2019, 8:18 AM
This revision now requires changes to proceed.Apr 2 2019, 8:18 AM
segfaultxavi commandeered this revision.Apr 2 2019, 10:44 AM
segfaultxavi edited reviewers, added: YOhoho; removed: segfaultxavi.

Coalescing fixes from comments and other revisions here.

I submitted another patch taking into account other diffs and their comments.

Yes, this patch does not only "improve the documentation", it defines a new behavior. This was the case initially, then I requested that the docs were updated to reflect the new behavior, and the docs patch ended up first in the patchset. This could be made clearer in the commit message, for sure.

Clarify commit message

I am also not too sure if we should speak about *items* here, since we provide abstractions for Items, but this here does not necessarily require items, rather just widgets...

segfaultxavi retitled this revision from efl.pack_linear: enhance documentation to efl.pack_linear: Clarify behavior and docs.Apr 3 2019, 1:56 AM
segfaultxavi edited the summary of this revision. (Show Details)

Add missing doc for unpack_at.

Clarify docs for unpack_at()

bu5hm4n accepted this revision.Apr 3 2019, 2:57 AM
This revision is now accepted and ready to land.Apr 3 2019, 2:57 AM
YOhoho requested changes to this revision.Apr 11 2019, 10:50 PM

It's wrong copy/paste.

src/lib/efl/interfaces/efl_pack_linear.eo
126–128

removed item.

This revision now requires changes to proceed.Apr 11 2019, 10:50 PM
segfaultxavi added inline comments.Apr 11 2019, 11:08 PM
src/lib/efl/interfaces/efl_pack_linear.eo
126–128

Sorry, I do not understand what you mean. What's wrong?

YOhoho added inline comments.Apr 11 2019, 11:41 PM
src/lib/efl/interfaces/efl_pack_linear.eo
126–128

to insert BEFORE is incorrect document for unpack_at.

Fix copy&paste error.

segfaultxavi marked 3 inline comments as done.Apr 12 2019, 12:04 AM

Fixed!

src/lib/efl/interfaces/efl_pack_linear.eo
126–128

Oh God! It's true!

YOhoho accepted this revision.Apr 12 2019, 12:07 AM

Thank you.

This revision is now accepted and ready to land.Apr 12 2019, 12:07 AM
Closed by commit rEFL058535528259: efl.pack_linear: Clarify behavior and docs (authored by Yeongjong Lee <yj34.lee@samsung.com>, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyApr 17 2019, 6:21 AM
This revision was automatically updated to reflect the committed changes.