Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Measuring the proper sizes of images in chat #15675

Merged
merged 4 commits into from
Apr 18, 2023
Merged

Measuring the proper sizes of images in chat #15675

merged 4 commits into from
Apr 18, 2023

Conversation

alwx
Copy link
Contributor

@alwx alwx commented Apr 18, 2023

Fixes #15654

Platforms

  • Android
  • iOS

Areas that maybe impacted

Functional
  • 1-1 chats
  • public chats
  • group chats

status: ready

@status-im-auto
Copy link
Member

status-im-auto commented Apr 18, 2023

Jenkins Builds

Click to see older builds (4)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ f49efb2 #1 2023-04-18 08:44:34 ~5 min android-e2e 🤖apk 📲
✔️ f49efb2 #1 2023-04-18 08:45:03 ~6 min tests 📄log
✔️ f49efb2 #1 2023-04-18 08:45:56 ~7 min android 🤖apk 📲
✔️ f49efb2 #1 2023-04-18 08:46:26 ~7 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 995180c #2 2023-04-18 10:07:04 ~5 min android-e2e 🤖apk 📲
✔️ 995180c #2 2023-04-18 10:07:51 ~6 min android 🤖apk 📲
✔️ 995180c #2 2023-04-18 10:08:03 ~6 min tests 📄log
✔️ 995180c #2 2023-04-18 10:09:49 ~8 min ios 📱ipa 📲
✔️ f3122e3 #3 2023-04-18 11:54:24 ~6 min android-e2e 🤖apk 📲
✔️ f3122e3 #3 2023-04-18 11:54:37 ~6 min tests 📄log
✔️ f3122e3 #3 2023-04-18 11:55:30 ~7 min android 🤖apk 📲
✔️ f3122e3 #3 2023-04-18 11:56:33 ~8 min ios 📱ipa 📲

true)]
(reset! measured-width width)
(reset! measured-height height)))
:style (style/image {:width @measured-width
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any safeguards in place if actual width and actual height are greater than our viewport?

Copy link
Contributor

@ibrkhalil ibrkhalil Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, Should we default to a factor of the screen dimensions ?
Like for example the max or the undefined value to be %25 width and height of the screen

Copy link
Contributor

@siddarthkay siddarthkay Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for small images and images with weird aspect ratios we will face scaling issues. I think better is to use object-fit css, something along the lines of this : https://stackoverflow.com/a/34713809 but then again I am not 100% sure of this solution either.

Will have to spend some time on this to get an optimal state.. @ibrkhalil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a safeguard, this is how the style for link-preview-loader is defined:

(defn image
  [{:keys [height width] :as dimensions}]
  (merge (if (and (pos? height)
                  (pos? width))
           (scale-dimensions dimensions)
           {:height 170})
         {:overflow      :hidden
          :border-radius 12}))

It uses the function scale-dimension which scales the pic and takes the screen width into the consideration (see status-im2.contexts.chat.messages.link-preview.style/scale-dimensions)

@alwx alwx requested a review from siddarthkay April 18, 2023 10:01
Copy link
Contributor

@siddarthkay siddarthkay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@qoqobolo qoqobolo self-assigned this Apr 18, 2023
@status-im-auto
Copy link
Member

93% of end-end tests have passed

Total executed tests: 29
Failed tests: 2
Passed tests: 27
IDs of failed tests: 702850,702838 

Failed tests (2)

Click to expand
  • Rerun failed tests

  • Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_activity_center_contact_request_decline, id: 702850

    Device 2: Find `Button` by `accessibility id`: `tab-recent`
    Device 2: Tap on found: Button

    medium/test_activity_center.py:142: in test_activity_center_contact_request_decline
        self.errors.verify_no_errors()
    base_test_case.py:184: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Username is not shown on 'Add contact' page after entering valid public key 
    

    [[Blocked by 15500]]

    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_message_send_check_timestamps_sender_username, id: 702838

    Device 2: Verifying that 'hello' is under today
    Device 2: Looking for a message by text: hello

    critical/test_public_chat_browsing.py:418: in test_community_message_send_check_timestamps_sender_username
        channel.verify_message_is_under_today_text(message, self.errors)
    ../views/chat_view.py:923: in verify_message_is_under_today_text
        message_element.wait_for_visibility_of_element()
    ../views/base_element.py:135: in wait_for_visibility_of_element
        raise TimeoutException(
     Device 2: ChatElementByText by xpath:`//*[starts-with(@text,'hello')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']` is not found on the screen after wait_for_visibility_of_element 
    

    [[blocked by 14797]]

    Device sessions

    Passed tests (27)

    Click to expand

    Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_activity_center_contact_request_accept_swipe_mark_all_as_read, id: 702851
    Device sessions

    Class TestActivityMultipleDevicePR:

    1. test_activity_center_reply_read_unread_delete_filter_swipe, id: 702947
    Device sessions

    2. test_activity_center_admin_notification_accept_swipe, id: 702958
    Device sessions

    3. test_navigation_jump_to, id: 702936
    Device sessions

    4. test_activity_center_mentions, id: 702957
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_text_message_delete_push_disappear, id: 702733
    Device sessions

    2. test_1_1_chat_edit_message, id: 702855
    Device sessions

    3. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    4. test_1_1_chat_message_reaction, id: 702730
    Device sessions

    5. test_1_1_chat_emoji_send_reply_and_open_link, id: 702782
    Device sessions

    6. test_1_1_chat_is_shown_message_sent_delivered_from_offline, id: 702783
    Device sessions

    7. test_1_1_chat_pin_messages, id: 702731
    Device sessions

    8. test_1_1_chat_push_emoji, id: 702813
    Device sessions

    9. test_1_1_chat_delete_via_long_press_relogin, id: 702784
    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_emoji_send_copy_paste_reply, id: 702840
    Device sessions

    2. test_community_links_with_previews_github_youtube_twitter_gif_send_enable, id: 702844
    Device sessions

    3. test_community_mentions_push_notification, id: 702786
    Device sessions

    4. test_community_contact_block_unblock_offline, id: 702894
    Device sessions

    5. test_community_leave, id: 702845
    Device sessions

    6. test_community_unread_messages_badge, id: 702841
    Device sessions

    7. test_community_message_delete, id: 702839
    Device sessions

    8. test_community_message_edit, id: 702843
    Device sessions

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_pin_messages, id: 702732
    Device sessions

    2. test_group_chat_join_send_text_messages_push, id: 702807
    Device sessions

    3. test_group_chat_offline_pn, id: 702808
    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_community_navigate_to_channel_when_relaunch, id: 702846
    Device sessions

    2. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    @qoqobolo
    Copy link
    Contributor

    Thanks for your work @alwx!
    PR is tested and can be merged.

    @alwx alwx merged commit a0f2481 into develop Apr 18, 2023
    @alwx alwx deleted the bugfix/15654 branch April 18, 2023 14:56
    ibrkhalil pushed a commit that referenced this pull request Apr 20, 2023
    * Measuring the proper sizes of images
    
    * Style fix
    alwx added a commit that referenced this pull request Apr 28, 2023
    * Measuring the proper sizes of images
    
    * Style fix
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    No open projects
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    Vertical gifs are cropped on mobile devices
    5 participants