Added the feature to display the external link in the warning dialog box #3193

shankarpriyank · 2023-01-07T10:25:17Z

Made changes to files related to Dialog Boxes, to show the URL of external link in the warning dialog box

Screenshots

Light mode	Dark mode

kelson42 · 2023-01-07T10:36:24Z

Few remarks:

I would remove the "Link - "
Buttons should be better aligned, "Yes" and "no" aligned left and on a first ligne, the "Don't ask anymore" left aligned on a second lign

shankarpriyank · 2023-01-07T10:48:29Z

Okay, willl remove "Link-"
Sorry but I cannot comprehend what do you mean by

"Yes" and "no" aligned left and on a first ligne, the "Don't ask anymore" left aligned on a second lign

I guess all there buttons are on three different lines, also I did not did not change anything related to the buttons.

This is how the dialog box looks on the develop branch

kelson42 · 2023-01-07T10:53:02Z

@shankarpriyank Don't remove the link, remove "Link -" label. We know this is a lonk, not needed to explain it.

shankarpriyank · 2023-01-07T17:15:27Z

Hey @kelson42 I have removed the label, and also fixed the failing tests.
Thought the lint checks will still fail, the error I am getting is this
This expression contains a magic number. Consider defining it to a well named constant. [MagicNumber]
This is thrown at a line where I am assigning a value to a variable, I am unable to understand how to fix this.

kelson42 · 2023-01-07T17:17:04Z

@shankarpriyank Thank you, @gouri-panda will review your work. Give him a few days.

shankarpriyank · 2023-01-07T17:19:03Z

Updated Screenshot

gouri-panda · 2023-01-07T18:24:07Z

This expression contains a magic number. Consider defining it to a well named constant. [MagicNumber]
This is thrown at a line where I am assigning a value to a variable, I am unable to understand how to fix this.

@shankarpriyank You can suppress this by annotating method supress warnings or make a constant value.

shankarpriyank · 2023-01-07T18:36:28Z

This expression contains a magic number. Consider defining it to a well named constant. [MagicNumber]
This is thrown at a line where I am assigning a value to a variable, I am unable to understand how to fix this.

@shankarpriyank You can suppress this by annotating method supress warnings or make a constant value.

Thanks, I understand the supressing alternative, but what do you mean by " make a constant value?"

Also can you please help me with the failing tests, I don't have much experience in testing,but after having a look at the log it seems to me like that the mock objects in some ways different to the real one.

gouri-panda · 2023-01-07T18:44:10Z

what do you mean by > make a constant value

Constant value means create a constant variable. The error is simply means you can't put only numbers in arguments. By doing that we can put different number in different locations by mistake. In order to not make this mistake we can put a constant variable and use it everywhere else.

shankarpriyank · 2023-01-07T18:47:39Z

I guess I have done that same thing

private val viewSpacingLeftForLink = 0
  private val viewSpacingRightForLink = 0
  private val viewSpacingTopForLink = 10
  private val viewSpacingBottomForLink = 0

I declared these variables and then passed them as arguments in this function

         setView(
            textView,
            viewSpacingLeftForLink,
            viewSpacingTopForLink,
            viewSpacingRightForLink,
            viewSpacingBottomForLink
          )

core/src/main/java/org/kiwix/kiwixmobile/core/utils/dialog/AlertDialogShower.kt

gouri-panda · 2023-01-08T19:02:33Z

@kelson42 Are you ok with the UI?

kelson42 · 2023-01-08T19:15:01Z

@gouri-panda AFAIK the UI has not been fixed, so would like to see a proper UI, the current one buttons are not properly aligned.

gouri-panda · 2023-01-08T19:18:16Z

@kelson42 Ok. @shankarpriyank Please change the UI as @kelson42 mentioned. Then I'll review the other parts :) Also, the other errors haven't been fixed yet.

kelson42 · 2023-01-11T13:21:05Z

If the alignment problen can not be fixed, please open a dedicated ticket. Then we could finally review and merge this PR.

kelson42 · 2023-01-11T19:34:06Z

It should be also confirmed that #2966 has been fixed.

gouri-panda · 2023-01-14T09:42:49Z

@shankarpriyank This test is failing.

ExternalLinkOpenerTest > alertDialogShower does not open link if negative-button is clicked$core_debug() FAILED
io.mockk.MockKException at ExternalLinkOpenerTest.kt:68

The ExternalLinkOpener has changed. If the change is intentional then you can modify these tests. If you want more tests that you have changed you can add more 🙂

kelson42 · 2023-01-17T08:02:44Z

@MohitMaliDeveloper Can you please complete the work here so we can move ahead?

shankarpriyank · 2023-01-17T08:19:42Z

Hey @kelson42 I am still working on modifying the tests so that they pass. I donot have much experience in testing and this is the first time I am encountering mocks so its a bit challenging. I could use some help to fix the tests.

kelson42 · 2023-01-17T08:21:01Z

@shankarpriyank Sorry, my bad, I had the feeling you were in a dead-end.

core/src/main/java/org/kiwix/kiwixmobile/core/utils/dialog/AlertDialogShower.kt

4shutosh · 2023-01-22T18:28:33Z

core/src/main/java/org/kiwix/kiwixmobile/core/utils/dialog/AlertDialogShower.kt

+            ).show()
+            true
+          }
+          textView.text = Html.fromHtml("</br><a href=$url> <b>$url</b>")


Suggestion: Instead of converting a link to html text, you can always use spannable string, and implement a custom click behaviour here, instead of leaving it to OS.

kelson42 · 2023-01-26T05:52:35Z

What is the status here?

shankarpriyank · 2023-01-27T07:05:18Z

The functionality has been implemented, but I am unable to modify the tests so that they pass. Took some help from @4shutosh but still no success.

kelson42 · 2023-01-27T07:07:38Z

@MohitMaliDeveloper Any chance you can complete and review his PR?

MohitMaliFtechiz · 2023-01-27T13:46:05Z

@MohitMaliDeveloper Any chance you can complete and review his PR?

ok

codecov · 2023-01-28T12:24:54Z

Codecov Report

Base: 49.68% // Head: 49.63% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (44a8892) compared to base (cafd6d7).
Patch coverage: 65.71% of modified lines in pull request are covered.

❗ Current head 44a8892 differs from pull request most recent head 4af50e3. Consider uploading reports for the commit 4af50e3 to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #3193      +/-   ##
=============================================
- Coverage      49.68%   49.63%   -0.05%     
  Complexity        25       25              
=============================================
  Files            290      291       +1     
  Lines           9800     9834      +34     
  Branches        1309     1311       +2     
=============================================
+ Hits            4869     4881      +12     
- Misses          4266     4282      +16     
- Partials         665      671       +6

Impacted Files	Coverage Δ
...kiwix/kiwixmobile/core/utils/ExternalLinkOpener.kt	`36.66% <50.00%> (-32.30%)`	⬇️
...iwix/kiwixmobile/core/utils/dialog/DialogShower.kt	`50.00% <50.00%> (ø)`
...kiwixmobile/core/utils/dialog/AlertDialogShower.kt	`73.68% <67.74%> (-8.46%)`	⬇️
.../org/kiwix/kiwixmobile/language/viewmodel/State.kt	`93.54% <0.00%> (-3.23%)`	⬇️
...rg/kiwix/kiwixmobile/core/search/SearchFragment.kt	`62.22% <0.00%> (-1.12%)`	⬇️
.../kiwix/kiwixmobile/core/main/CoreReaderFragment.kt	`30.48% <0.00%> (-0.12%)`	⬇️
...x/kiwixmobile/core/extensions/ContextExtensions.kt	`50.00% <0.00%> (+13.63%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kelson42 · 2023-01-28T12:25:36Z

@MohitMaliFtechiz Have you finished the necessary work? Shoukd you remove the draft mode?

MohitMaliFtechiz · 2023-01-28T12:36:25Z

@MohitMaliFtechiz Have you finished the necessary work? Shoukd you remove the draft mode?

@kelson42 ,
I have fixed test and lint failures.

AFAIK the UI has not been fixed, so would like to see a proper UI, the current one buttons are not properly aligned.

Ui changes are left.

kelson42 · 2023-01-28T12:37:33Z

@MohitMaliFtechiz OK, who will do that?

MohitMaliFtechiz · 2023-01-28T12:40:08Z

@MohitMaliFtechiz OK, who will do that?

@kelson42 , I'm working on it.

MohitMaliFtechiz · 2023-01-28T13:14:56Z

@kelson42 ,
I'm testing the UI. this is a AlertDialog and it's button automatically changed according to every screen size and how many space left in screen.

Example :-

In Portrait Mode

In Landscape Mode

If we want to change the UI then we need to use a custom layout with Custom Dialog instead of Android AlertDialog (which we are currently using).

kelson42 · 2023-01-28T14:24:32Z

@MohitMaliDeveloper Not too bad, can we reduce the big white space? Please rebase the PR as well.

MohitMaliFtechiz · 2023-01-30T09:39:33Z

@MohitMaliDeveloper Not too bad, can we reduce the big white space?

@kelson42 , unfortunately not, it's automatically takes height according to it's views (when we call setView method of alertDialog for adding additional view to it. in this case we are adding url text view).

Please rebase the PR as well.

I have re base the PR and resolved the conflicts.

kelson42 · 2023-02-01T06:05:29Z

@MohitMaliDeveloper Is that ready to review (please rebase and fix conflicts)?

MohitMaliFtechiz · 2023-02-01T07:03:52Z

@MohitMaliDeveloper Is that ready to review (please rebase and fix conflicts)?

@kelson42 , i'm waiting for your feedback on above comment. that's why this PR in draft, if that is okay for you then this PR is ready for review.

kelson42 · 2023-02-01T07:48:53Z

@MohitMaliFtechiz LGTM. If you request something from me, please specificaly ask with a question mark.

MohitMaliFtechiz · 2023-02-02T07:03:01Z

@kelson42 , I have resolved the conflicts. @gouri-panda this PR is ready for review.

gouri-panda · 2023-02-02T10:09:43Z

@MohitMaliDeveloper Thanks! Will do!

gouri-panda

@MohitMaliDeveloper @shankarpriyank I overviewed the PR. I leave some comments about this. The external link colour doesn't look good on the dark theme. Just a small friendly reminder, please don't change the unrelated code to tickets as you did here, changing the licence and renaming the function names.

core/src/main/java/org/kiwix/kiwixmobile/core/utils/dialog/AlertDialogShower.kt

MohitMaliFtechiz · 2023-02-03T06:25:53Z

@gouri-panda , thanks for your feedback.

The external link colour doesn't look good on the dark theme.

Good catch. I have changed the color of link and update the new UI screenshots on PR description.

changing the licence

It's automatically changed when we move file to another package. I'm reverting the Copyright (c) to 2020.

renaming the function names

Now ExternalLinkOpenerTest file is in androidTest package, here these function names are not allowed that's why i had changed it.

gouri-panda · 2023-02-03T09:22:41Z

@MohitMaliDeveloper Did you push it? I don't see recent changes.

MohitMaliFtechiz · 2023-02-03T09:36:08Z

@MohitMaliDeveloper Did you push it? I don't see recent changes.

hi @gouri-panda ,
I have just push the all latest changes(as you requested).

gouri-panda

@shankarpriyank @MohitMaliDeveloper Thanks for the work 🎉 !

…ethod and improved code

kelson42 requested a review from gouri-panda January 7, 2023 17:16

shankarpriyank commented Jan 7, 2023

View reviewed changes

core/src/main/java/org/kiwix/kiwixmobile/core/utils/dialog/AlertDialogShower.kt Outdated Show resolved Hide resolved

shankarpriyank mentioned this pull request Jan 11, 2023

Ability to copy the URL of an external link #2966

Closed

4shutosh reviewed Jan 22, 2023

View reviewed changes

core/src/main/java/org/kiwix/kiwixmobile/core/utils/dialog/AlertDialogShower.kt Outdated Show resolved Hide resolved

4shutosh reviewed Jan 22, 2023

View reviewed changes

MohitMaliFtechiz marked this pull request as draft January 28, 2023 12:23

MohitMaliFtechiz force-pushed the fix_urlInWarningWindow branch from 71a9314 to 711c719 Compare February 2, 2023 06:27

MohitMaliFtechiz marked this pull request as ready for review February 2, 2023 07:00

gouri-panda reviewed Feb 2, 2023

View reviewed changes

core/src/main/java/org/kiwix/kiwixmobile/core/utils/dialog/AlertDialogShower.kt Outdated Show resolved Hide resolved

kelson42 requested a review from gouri-panda February 3, 2023 07:14

gouri-panda approved these changes Feb 3, 2023

View reviewed changes

shankarpriyank and others added 7 commits February 3, 2023 12:06

Added the feature to display the external link in the warning dialog box

692176f

Remove Label From Dialog Box

fe459e6

Implemented LongClick Functionality

fac6c13

Tweaked Tests and suppressed warnings

c63c866

Fixed CI and lint failure

3146ba8

Fixed Unexpected indentation

426093b

Fixed RestrictedApi and MagicNumber lint errors, removed deprecated m…

4af50e3

…ethod and improved code

kelson42 force-pushed the fix_urlInWarningWindow branch from 44a8892 to 4af50e3 Compare February 3, 2023 11:06

kelson42 merged commit 0d1593f into kiwix:develop Feb 3, 2023

Added the feature to display the external link in the warning dialog box #3193

Added the feature to display the external link in the warning dialog box #3193

Conversation

shankarpriyank commented Jan 7, 2023 • edited by MohitMaliFtechiz

kelson42 commented Jan 7, 2023

shankarpriyank commented Jan 7, 2023

kelson42 commented Jan 7, 2023

shankarpriyank commented Jan 7, 2023

kelson42 commented Jan 7, 2023

shankarpriyank commented Jan 7, 2023

gouri-panda commented Jan 7, 2023

shankarpriyank commented Jan 7, 2023 • edited

gouri-panda commented Jan 7, 2023

shankarpriyank commented Jan 7, 2023

gouri-panda commented Jan 8, 2023

kelson42 commented Jan 8, 2023

gouri-panda commented Jan 8, 2023

kelson42 commented Jan 11, 2023

kelson42 commented Jan 11, 2023

gouri-panda commented Jan 14, 2023

kelson42 commented Jan 17, 2023

shankarpriyank commented Jan 17, 2023

kelson42 commented Jan 17, 2023

4shutosh Jan 22, 2023

Choose a reason for hiding this comment

kelson42 commented Jan 26, 2023

shankarpriyank commented Jan 27, 2023

kelson42 commented Jan 27, 2023

MohitMaliFtechiz commented Jan 27, 2023

codecov bot commented Jan 28, 2023 • edited

Codecov Report

kelson42 commented Jan 28, 2023

MohitMaliFtechiz commented Jan 28, 2023

kelson42 commented Jan 28, 2023

MohitMaliFtechiz commented Jan 28, 2023 • edited

MohitMaliFtechiz commented Jan 28, 2023 • edited

kelson42 commented Jan 28, 2023

MohitMaliFtechiz commented Jan 30, 2023 • edited

kelson42 commented Feb 1, 2023

MohitMaliFtechiz commented Feb 1, 2023

kelson42 commented Feb 1, 2023

MohitMaliFtechiz commented Feb 2, 2023

gouri-panda commented Feb 2, 2023

gouri-panda left a comment

Choose a reason for hiding this comment

MohitMaliFtechiz commented Feb 3, 2023 • edited

gouri-panda commented Feb 3, 2023

MohitMaliFtechiz commented Feb 3, 2023

gouri-panda left a comment

Choose a reason for hiding this comment

shankarpriyank commented Jan 7, 2023 •

edited by MohitMaliFtechiz

shankarpriyank commented Jan 7, 2023 •

edited

codecov bot commented Jan 28, 2023 •

edited

MohitMaliFtechiz commented Jan 28, 2023 •

edited

MohitMaliFtechiz commented Jan 28, 2023 •

edited

MohitMaliFtechiz commented Jan 30, 2023 •

edited

MohitMaliFtechiz commented Feb 3, 2023 •

edited